diff --git a/apps/dashboard/package.json b/apps/dashboard/package.json index 115d020..30c3e93 100644 --- a/apps/dashboard/package.json +++ b/apps/dashboard/package.json @@ -34,6 +34,7 @@ "@tanstack/react-router-devtools": "~1.166.11", "@tanstack/react-router-ssr-query": "~1.166.10", "@tanstack/react-start": "~1.167.23", + "@tanstack/react-virtual": "^3.13.24", "@tanstack/router-plugin": "~1.167.12", "agentation": "^3.0.2", "better-auth": "^1.6.0", diff --git a/apps/dashboard/src/components/pulls/review/review-diff-pane.tsx b/apps/dashboard/src/components/pulls/review/review-diff-pane.tsx index 1b5b6ca..1a16d29 100644 --- a/apps/dashboard/src/components/pulls/review/review-diff-pane.tsx +++ b/apps/dashboard/src/components/pulls/review/review-diff-pane.tsx @@ -8,6 +8,7 @@ import { useEffect, useImperativeHandle, useMemo, + useReducer, useRef, useState, } from "react"; @@ -257,91 +258,103 @@ export const ReviewDiffPane = memo( [files, visibleCount], ); - // Single shared observer for active file tracking + // Near-viewport tracking lives in a ref so the observer is NOT torn + // down on every scroll tick. State updates to a Set dep previously + // recreated the observer, opening a race window where elements that + // scrolled into view during teardown were never re-observed — the + // "diffs sometimes don't render" bug. + const nearViewportRef = useRef>(new Set()); + const [, bumpNearViewport] = useReducer((n: number) => n + 1, 0); + const nearViewportObserverRef = useRef(null); + const activeFileObserverRef = useRef(null); + const onActiveFileChangeRef = useRef(onActiveFileChange); + + useEffect(() => { + onActiveFileChangeRef.current = onActiveFileChange; + }, [onActiveFileChange]); + + // Create both observers once, tied to the scroll panel's lifetime. useEffect(() => { const panel = diffPanelRef.current; - if (!panel || visibleFiles.length === 0) return; + if (!panel) return; - const observer = new IntersectionObserver( + const nearObserver = new IntersectionObserver( (entries) => { + let changed = false; for (const entry of entries) { if (!entry.isIntersecting) continue; - const filename = entry.target.getAttribute("data-filename"); - if (filename) { - onActiveFileChange(filename); + if (filename && !nearViewportRef.current.has(filename)) { + nearViewportRef.current.add(filename); + nearObserver.unobserve(entry.target); + changed = true; } } + if (changed) bumpNearViewport(); }, { root: panel, - rootMargin: "-10% 0px -80% 0px", + rootMargin: "1500px 0px", threshold: 0, }, ); - for (const file of visibleFiles) { - const element = document.getElementById(encodeFileId(file.filename)); - if (element) observer.observe(element); - } - - return () => observer.disconnect(); - }, [onActiveFileChange, visibleFiles]); - - // Single shared observer for viewport proximity — controls when diff content mounts - const [nearViewportFiles, setNearViewportFiles] = useState>( - () => new Set(), - ); - - // Seed the first visible files as near-viewport immediately. - // During client-side navigation the scroll container may not have its - // final dimensions when the IntersectionObserver first checks, causing - // all diffs to remain as empty placeholders until a hard refresh. - useEffect(() => { - if (visibleFiles.length === 0 || nearViewportFiles.size > 0) return; - setNearViewportFiles( - new Set(visibleFiles.slice(0, 4).map((f) => f.filename)), - ); - }, [visibleFiles, nearViewportFiles.size]); - - useEffect(() => { - const panel = diffPanelRef.current; - if (!panel || visibleFiles.length === 0) return; - - const observer = new IntersectionObserver( + const activeObserver = new IntersectionObserver( (entries) => { - const newlyVisible: string[] = []; for (const entry of entries) { if (!entry.isIntersecting) continue; const filename = entry.target.getAttribute("data-filename"); - if (filename) { - newlyVisible.push(filename); - observer.unobserve(entry.target); - } - } - if (newlyVisible.length > 0) { - setNearViewportFiles((prev) => { - const next = new Set(prev); - for (const f of newlyVisible) next.add(f); - return next; - }); + if (filename) onActiveFileChangeRef.current(filename); } }, { root: panel, - rootMargin: "1500px 0px", + rootMargin: "-10% 0px -80% 0px", threshold: 0, }, ); + nearViewportObserverRef.current = nearObserver; + activeFileObserverRef.current = activeObserver; + + return () => { + nearObserver.disconnect(); + activeObserver.disconnect(); + nearViewportObserverRef.current = null; + activeFileObserverRef.current = null; + }; + }, []); + + // Seed the first visible files as near-viewport immediately. + // During client-side navigation the scroll container may not have its + // final dimensions when the IntersectionObserver first checks, causing + // all diffs to remain as empty placeholders until a hard refresh. + useEffect(() => { + if (visibleFiles.length === 0 || nearViewportRef.current.size > 0) { + return; + } + for (const file of visibleFiles.slice(0, 4)) { + nearViewportRef.current.add(file.filename); + } + bumpNearViewport(); + }, [visibleFiles]); + + // Observe any newly-mounted file elements. observe() is idempotent, so + // re-calling it on already-observed nodes is safe. + useEffect(() => { + const nearObserver = nearViewportObserverRef.current; + const activeObserver = activeFileObserverRef.current; + if (!nearObserver || !activeObserver) return; + for (const file of visibleFiles) { - if (nearViewportFiles.has(file.filename)) continue; const element = document.getElementById(encodeFileId(file.filename)); - if (element) observer.observe(element); + if (!element) continue; + activeObserver.observe(element); + if (!nearViewportRef.current.has(file.filename)) { + nearObserver.observe(element); + } } - - return () => observer.disconnect(); - }, [visibleFiles, nearViewportFiles]); + }, [visibleFiles]); if (totalFileCount === 0 && !hasNextPage) { return ( @@ -360,7 +373,7 @@ export const ReviewDiffPane = memo( id={encodeFileId(file.filename)} file={file} diffStyle={diffStyle} - isNearViewport={nearViewportFiles.has(file.filename)} + isNearViewport={nearViewportRef.current.has(file.filename)} annotations={ annotationsByFile.get(file.filename) ?? EMPTY_ANNOTATIONS } diff --git a/apps/dashboard/src/components/pulls/review/review-file-diff-block.tsx b/apps/dashboard/src/components/pulls/review/review-file-diff-block.tsx index c0b3abf..327ca50 100644 --- a/apps/dashboard/src/components/pulls/review/review-file-diff-block.tsx +++ b/apps/dashboard/src/components/pulls/review/review-file-diff-block.tsx @@ -7,6 +7,7 @@ import { import { toast } from "@diffkit/ui/components/sonner"; import { Spinner } from "@diffkit/ui/components/spinner"; import { quickhubDark, quickhubLight } from "@diffkit/ui/lib/diffs-themes"; +import { shikiBundledLangSet } from "@diffkit/ui/lib/shiki-bundle"; import { cn } from "@diffkit/ui/lib/utils"; import type { SelectedLineRange } from "@pierre/diffs"; import type { DiffLineAnnotation, PatchDiffProps } from "@pierre/diffs/react"; @@ -46,7 +47,7 @@ const DIFF_LINE_HEIGHT = 20; const DIFF_HUNK_SEPARATOR_HEIGHT = 28; function estimateDiffHeight( - patch: string | undefined, + patch: string | null | undefined, diffStyle: "unified" | "split", ): number { if (!patch) return 0; @@ -59,25 +60,55 @@ function estimateDiffHeight( ); } +// Kick off both Pierre chunks as soon as this module evaluates (i.e. when +// ReviewDiffPane loads) so the JS is already parsed by the time the first +// file block enters the viewport — no per-Suspense waterfall. +const patchDiffModulePromise: Promise<{ + default: ReviewPatchDiffComponent; +}> = import.meta.env.SSR + ? Promise.resolve({ default: (() => null) as ReviewPatchDiffComponent }) + : import("@pierre/diffs/react").then((mod) => ({ + default: mod.PatchDiff as ReviewPatchDiffComponent, + })); + +const pierreInitPromise: Promise = import.meta.env.SSR + ? Promise.resolve() + : import("@pierre/diffs").then( + ({ + registerCustomTheme, + EXTENSION_TO_FILE_FORMAT, + extendFileFormatMap, + }) => { + registerCustomTheme("quickhub-light", () => + Promise.resolve(quickhubLight), + ); + registerCustomTheme("quickhub-dark", () => + Promise.resolve(quickhubDark), + ); + + // Pierre's default map sends some extensions to grammars we deliberately + // excluded from our Shiki bundle (e.g. `.h` → `objective-cpp`). Shiki + // then throws "Language ... not found". Walk the default map and + // redirect any unsupported mapping to `text` so those diffs render + // without highlighting instead of failing. + const overrides: Record = {}; + for (const [ext, lang] of Object.entries(EXTENSION_TO_FILE_FORMAT)) { + if (!lang) continue; + if (lang === "text" || lang === "ansi") continue; + if (shikiBundledLangSet.has(lang)) continue; + overrides[ext] = "text"; + } + if (Object.keys(overrides).length > 0) { + extendFileFormatMap(overrides); + } + }, + ); +// Gate PatchDiff's Suspense resolution on pierreInit so the extension-map +// override is always in place before the first file's grammar is looked up. const PatchDiff: LazyExoticComponent = lazy(() => - import.meta.env.SSR - ? Promise.resolve({ - default: (() => null) as ReviewPatchDiffComponent, - }) - : import("@pierre/diffs/react").then((mod) => ({ - default: mod.PatchDiff as ReviewPatchDiffComponent, - })), + Promise.all([patchDiffModulePromise, pierreInitPromise]).then(([mod]) => mod), ); -let themesRegistered = false; -if (!import.meta.env.SSR && !themesRegistered) { - themesRegistered = true; - import("@pierre/diffs").then(({ registerCustomTheme }) => { - registerCustomTheme("quickhub-light", () => Promise.resolve(quickhubLight)); - registerCustomTheme("quickhub-dark", () => Promise.resolve(quickhubDark)); - }); -} - export const ReviewFileDiffBlock = memo(function ReviewFileDiffBlock({ id, file, @@ -127,6 +158,7 @@ export const ReviewFileDiffBlock = memo(function ReviewFileDiffBlock({ const [isCollapsed, setIsCollapsed] = useState(false); const { resolvedTheme } = useTheme(); const isDark = resolvedTheme === "dark"; + const handleGutterUtilityClick = useCallback( (range: SelectedLineRange) => onGutterClick(file.filename, range), [file.filename, onGutterClick], @@ -169,6 +201,19 @@ export const ReviewFileDiffBlock = memo(function ReviewFileDiffBlock({ file.changes <= LARGE_PATCH_CHANGE_THRESHOLD && (file.patch?.length ?? 0) <= LARGE_PATCH_CHAR_THRESHOLD; + const unsafeCSS = useMemo( + () => + [ + `:host { color-scheme: ${isDark ? "dark" : "light"}; }`, + `:host { --diffs-font-family: 'Geist Mono Variable', 'SF Mono', ui-monospace, 'Cascadia Code', monospace; }`, + `[data-line-annotation] { font-family: 'Inter Variable', 'Inter', 'Avenir Next', ui-sans-serif, system-ui, sans-serif; }`, + `[data-line-annotation] code { font-family: var(--diffs-font-family, var(--diffs-font-fallback)); }`, + `[data-annotation-content] { background-color: transparent; }`, + `[data-diff] { border: 1px solid var(--border); border-top: 0; border-radius: 0 0 4px 4px; overflow: hidden; }`, + ].join("\n"), + [isDark], + ); + const diffOptions = useMemo( () => ({ diffStyle, @@ -184,18 +229,96 @@ export const ReviewFileDiffBlock = memo(function ReviewFileDiffBlock({ enableGutterUtility: !readOnly, enableLineSelection: !readOnly, ...(readOnly ? {} : { onGutterUtilityClick: handleGutterUtilityClick }), - unsafeCSS: [ - `:host { color-scheme: ${isDark ? "dark" : "light"}; }`, - `:host { --diffs-font-family: 'Geist Mono Variable', 'SF Mono', ui-monospace, 'Cascadia Code', monospace; }`, - `[data-line-annotation] { font-family: 'Inter Variable', 'Inter', 'Avenir Next', ui-sans-serif, system-ui, sans-serif; }`, - `[data-line-annotation] code { font-family: var(--diffs-font-family, var(--diffs-font-fallback)); }`, - `[data-annotation-content] { background-color: transparent; }`, - `[data-diff] { border: 1px solid var(--border); border-top: 0; border-radius: 0 0 4px 4px; overflow: hidden; }`, - ].join("\n"), + unsafeCSS, }), - [diffStyle, handleGutterUtilityClick, isDark, readOnly, useWordDiff], + [diffStyle, handleGutterUtilityClick, readOnly, unsafeCSS, useWordDiff], ); const patchString = useMemo(() => buildPatchString(file), [file]); + const placeholderHeight = useMemo( + () => estimateDiffHeight(file.patch, diffStyle), + [file.patch, diffStyle], + ); + + const toggleCollapse = useCallback(() => setIsCollapsed((prev) => !prev), []); + + const renderAnnotation = useCallback( + (annotation: DiffLineAnnotation) => { + if (readOnly) return null; + + const data = annotation.metadata as + | PendingComment + | PullReviewComment + | null; + if (!data) return null; + + if ("body" in data && data.body === "__FORM__") { + const formData = data as PendingComment; + return ( + + onAddComment({ + path: file.filename, + line: formData.line, + startLine: formData.startLine, + side: formData.side, + startSide: formData.startSide, + body, + }) + } + onCancel={onCancelComment} + mentionConfig={mentionConfig} + /> + ); + } + + if ("body" in data && !("id" in data)) { + return ( + + ); + } + + if ("id" in data) { + const c = data as PullReviewComment; + const replies = repliesByCommentId.get(c.id); + return ( + + ); + } + + return null; + }, + [ + file.filename, + mentionConfig, + onAddComment, + onCancelComment, + onEditComment, + owner, + pullNumber, + readOnly, + repliesByCommentId, + repo, + threadInfoByCommentId, + viewerLogin, + ], + ); if (!file.patch) { return ( @@ -203,7 +326,7 @@ export const ReviewFileDiffBlock = memo(function ReviewFileDiffBlock({ setIsCollapsed(!isCollapsed)} + onToggleCollapse={toggleCollapse} /> {!isCollapsed && (
@@ -221,89 +344,20 @@ export const ReviewFileDiffBlock = memo(function ReviewFileDiffBlock({ setIsCollapsed(!isCollapsed)} + onToggleCollapse={toggleCollapse} /> {!isCollapsed && !isNearViewport && ( -
+
)} {!isCollapsed && isNearViewport && (
- + }> , - ) => { - if (readOnly) return null; - - const data = annotation.metadata as - | PendingComment - | PullReviewComment - | null; - if (!data) return null; - - if ("body" in data && data.body === "__FORM__") { - const formData = data as PendingComment; - return ( - - onAddComment({ - path: file.filename, - line: formData.line, - startLine: formData.startLine, - side: formData.side, - startSide: formData.startSide, - body, - }) - } - onCancel={onCancelComment} - mentionConfig={mentionConfig} - /> - ); - } - - if ("body" in data && !("id" in data)) { - return ( - - ); - } - - if ("id" in data) { - const c = data as PullReviewComment; - const replies = repliesByCommentId.get(c.id); - return ( - - ); - } - - return null; - }} + renderAnnotation={renderAnnotation} />
@@ -312,7 +366,7 @@ export const ReviewFileDiffBlock = memo(function ReviewFileDiffBlock({ ); }); -function FileHeader({ +const FileHeader = memo(function FileHeader({ file, isCollapsed, onToggleCollapse, @@ -392,7 +446,7 @@ function FileHeader({
); -} +}); function InlineCommentForm({ isMultiLine, diff --git a/apps/dashboard/src/components/pulls/review/review-file-tree.tsx b/apps/dashboard/src/components/pulls/review/review-file-tree.tsx index caa164a..5bdbf32 100644 --- a/apps/dashboard/src/components/pulls/review/review-file-tree.tsx +++ b/apps/dashboard/src/components/pulls/review/review-file-tree.tsx @@ -1,6 +1,14 @@ import { FileIcon, FolderIcon } from "@diffkit/icons"; import { cn } from "@diffkit/ui/lib/utils"; -import { memo, useCallback, useState, useSyncExternalStore } from "react"; +import { useVirtualizer } from "@tanstack/react-virtual"; +import { + memo, + useCallback, + useMemo, + useRef, + useState, + useSyncExternalStore, +} from "react"; import type { FileTreeNode } from "./review-types"; import { encodeFileId } from "./review-utils"; @@ -42,70 +50,169 @@ function useIsActiveFile(store: ActiveFileStore, path: string): boolean { return useSyncExternalStore(subscribe, getSnapshot); } -export const ReviewFileTreeNode = memo(function ReviewFileTreeNode({ - node, - depth, - activeFileStore, - onFileClick, -}: { +// Row height is fixed so useVirtualizer can compute absolute positions without +// measuring. Keep this in sync with the `py-1.5 text-[13px]` styling below. +const ROW_HEIGHT = 30; + +type FlatRow = { node: FileTreeNode; depth: number; - activeFileStore: ActiveFileStore; - onFileClick: (path: string) => void; -}) { - const [isOpen, setIsOpen] = useState(true); +}; + +function collectFlatRows( + nodes: FileTreeNode[], + expanded: ReadonlySet, + depth: number, + out: FlatRow[], +) { + for (const node of nodes) { + out.push({ node, depth }); + if (node.type === "directory" && expanded.has(node.path)) { + collectFlatRows(node.children, expanded, depth + 1, out); + } + } +} + +function collectAllDirectoryPaths( + nodes: FileTreeNode[], + out: Set, +): Set { + for (const node of nodes) { + if (node.type === "directory") { + out.add(node.path); + collectAllDirectoryPaths(node.children, out); + } + } + return out; +} + +export const ReviewVirtualizedFileTree = memo( + function ReviewVirtualizedFileTree({ + tree, + activeFileStore, + onFileClick, + }: { + tree: FileTreeNode[]; + activeFileStore: ActiveFileStore; + onFileClick: (path: string) => void; + }) { + // Default: every directory expanded. Once the user toggles something we + // store their explicit override in a state Set. + const defaultExpanded = useMemo( + () => collectAllDirectoryPaths(tree, new Set()), + [tree], + ); + const [explicitExpanded, setExplicitExpanded] = + useState | null>(null); + const expanded = explicitExpanded ?? defaultExpanded; + + const flatRows = useMemo(() => { + const out: FlatRow[] = []; + collectFlatRows(tree, expanded, 0, out); + return out; + }, [tree, expanded]); + + const toggleDirectory = useCallback( + (path: string) => { + setExplicitExpanded((prev) => { + const base = prev ?? defaultExpanded; + const next = new Set(base); + if (next.has(path)) next.delete(path); + else next.add(path); + return next; + }); + }, + [defaultExpanded], + ); + + const scrollRef = useRef(null); + + const virtualizer = useVirtualizer({ + count: flatRows.length, + getScrollElement: () => scrollRef.current, + estimateSize: () => ROW_HEIGHT, + overscan: 12, + }); + + const items = virtualizer.getVirtualItems(); + const totalSize = virtualizer.getTotalSize(); - if (node.type === "directory") { return ( -
- - {isOpen && ( -
- {node.children.map((child) => ( - - ))} -
- )} +
+
+ {items.map((virtualItem) => { + const row = flatRows[virtualItem.index]; + if (!row) return null; + const { node, depth } = row; + return ( +
+ {node.type === "directory" ? ( + + ) : ( + + )} +
+ ); + })} +
); - } + }, +); +const DirectoryRow = memo(function DirectoryRow({ + node, + depth, + isOpen, + onToggle, +}: { + node: FileTreeNode; + depth: number; + isOpen: boolean; + onToggle: (path: string) => void; +}) { return ( - + ); }); @@ -127,7 +234,7 @@ const FileTreeLeaf = memo(function FileTreeLeaf({ { + if ( + fileSummariesQuery.hasNextPage && + !fileSummariesQuery.isFetchingNextPage + ) { + void fileSummariesQuery.fetchNextPage(); + } + }, [ + fileSummariesQuery.hasNextPage, + fileSummariesQuery.isFetchingNextPage, + fileSummariesQuery.fetchNextPage, + ]); + const filesQuery = useInfiniteQuery({ queryKey: filesQueryKey, initialPageParam: 1, @@ -164,6 +182,20 @@ export function ReviewPage() { refetchOnWindowFocus: false, }); + // Auto-fetch remaining pages of patches in the background so that by the + // time the user scrolls deeper into the PR, the data is already there. + // Keeps the initial perPage small so the first-visible files render fast, + // then streams the rest chunk-by-chunk behind the UI. + useEffect(() => { + if (filesQuery.hasNextPage && !filesQuery.isFetchingNextPage) { + void filesQuery.fetchNextPage(); + } + }, [ + filesQuery.hasNextPage, + filesQuery.isFetchingNextPage, + filesQuery.fetchNextPage, + ]); + const hasDiffPayload = filesQuery.data !== undefined; const reviewCommentsQuery = useQuery({ ...githubPullReviewCommentsQueryOptions(scope, input), @@ -178,7 +210,10 @@ export function ReviewPage() { useGitHubSignalStream(webhookRefreshTargets); const pr = pageQuery.data?.detail ?? null; - const sidebarFiles = fileSummariesQuery.data ?? []; + const sidebarFiles = useMemo( + () => fileSummariesQuery.data?.pages.flatMap((page) => page.items) ?? [], + [fileSummariesQuery.data], + ); const diffFiles = useMemo( () => filesQuery.data?.pages.flatMap((page) => page.files) ?? [], [filesQuery.data], @@ -248,7 +283,7 @@ export function ReviewPage() { mentionActivated, ]); - const [diffStyle, setDiffStyle] = useState<"unified" | "split">("unified"); + const [diffStyle, setDiffStyle] = useState<"unified" | "split">("split"); const [pendingComments, setPendingComments] = useState([]); const [activeCommentForm, setActiveCommentForm] = useState(null); @@ -339,15 +374,14 @@ export function ReviewPage() { return map; }, [pendingComments]); - const diffStats = useMemo(() => { - let totalAdditions = 0; - let totalDeletions = 0; - for (const file of sidebarFiles) { - totalAdditions += file.additions; - totalDeletions += file.deletions; - } - return { totalAdditions, totalDeletions }; - }, [sidebarFiles]); + // Read totals off the PR node (one cheap GraphQL call) instead of summing + // streamed file summaries — otherwise the toolbar chip can't show real + // numbers until every summary page arrives (~seconds for a 3k-file PR). + const diffStats = { + totalAdditions: pr?.additions ?? 0, + totalDeletions: pr?.deletions ?? 0, + }; + const prChangedFiles = pr?.changedFiles ?? 0; const addPendingComment = useCallback((comment: PendingComment) => { setPendingComments((previous) => [...previous, comment]); @@ -441,8 +475,28 @@ export function ReviewPage() { if (success) { setPendingComments([]); + // Only invalidate queries scoped to this specific PR. Invalidating + // `githubQueryKeys.all` would refetch every GitHub query in the + // client (other repos, notifications, signals) for no reason. void queryClient.invalidateQueries({ - queryKey: githubQueryKeys.all, + predicate: (query) => { + const key = query.queryKey; + if (key[0] !== "github") return false; + for (let i = 2; i < key.length; i++) { + const part = key[i]; + if (!part || typeof part !== "object") continue; + const rec = part as Record; + if ( + rec.owner === owner && + rec.repo === repo && + (rec.pullNumber === pullNumber || + rec.issueNumber === pullNumber) + ) { + return true; + } + } + return false; + }, }); void navigate({ to: "/$owner/$repo/pull/$pullId", @@ -501,6 +555,8 @@ export function ReviewPage() { } const sidebarFileCount = sidebarFiles.length; + const isStreamingSummaries = + fileSummariesQuery.isFetching || fileSummariesQuery.hasNextPage; const diffContent = hasDiffPayload ? ( }> @@ -541,7 +597,7 @@ export function ReviewPage() { repo={repo} pullId={pullId} pr={pr} - sidebarFileCount={sidebarFileCount} + sidebarFileCount={prChangedFiles} diffStats={diffStats} diffStyle={diffStyle} onSetDiffStyle={setDiffStyle} @@ -558,6 +614,7 @@ export function ReviewPage() { @@ -576,6 +633,7 @@ export function ReviewPage() { @@ -722,11 +780,13 @@ const ReviewToolbar = memo(function ReviewToolbar({ export const ReviewSidebar = memo(function ReviewSidebar({ sidebarFiles, sidebarFileCount, + isStreamingSummaries = false, activeFileStore, onFileClick, }: { sidebarFiles: PullFileSummary[]; sidebarFileCount: number; + isStreamingSummaries?: boolean; activeFileStore: ActiveFileStore; onFileClick: (path: string) => void; }) { @@ -768,20 +828,23 @@ export const ReviewSidebar = memo(function ReviewSidebar({ shortcutKey="f" /> -
- {fileTree.map((node) => ( - - ))} -
+ -
- {sidebarFileCount} {sidebarFileCount === 1 ? "file" : "files"} changed +
+ + {sidebarFileCount} {sidebarFileCount === 1 ? "file" : "files"} + {isStreamingSummaries ? " loaded…" : " changed"} + + {isStreamingSummaries && ( + + )}
); diff --git a/apps/dashboard/src/components/repo/commit-diff-pane.tsx b/apps/dashboard/src/components/repo/commit-diff-pane.tsx index d13f124..53acfab 100644 --- a/apps/dashboard/src/components/repo/commit-diff-pane.tsx +++ b/apps/dashboard/src/components/repo/commit-diff-pane.tsx @@ -7,6 +7,7 @@ import { useEffect, useImperativeHandle, useMemo, + useReducer, useRef, useState, } from "react"; @@ -25,6 +26,12 @@ const EMPTY_ANNOTATIONS: DiffLineAnnotation[] = []; const EMPTY_PENDING_COMMENTS: PendingComment[] = []; const EMPTY_REPLIES = new Map(); const NOOP_COMMENT_FORM: ActiveCommentForm | null = null; +// Hoist noop callbacks to module scope so their identity is stable across +// renders. Inline arrow props bust ReviewFileDiffBlock's memo on every render. +const noopStartComment = (_filename: string, _range: SelectedLineRange) => {}; +const noopCancel = () => {}; +const noopAdd = (_comment: PendingComment) => {}; +const noopEdit = (_original: PendingComment, _newBody: string) => {}; export type CommitDiffPaneHandle = { scrollToFile: (filename: string) => void; @@ -176,85 +183,95 @@ export const CommitDiffPane = memo( [files, visibleCount], ); + // Same observer-stability pattern as ReviewDiffPane: observers live in + // refs so they survive state updates; otherwise listing the state they + // write in the effect's deps tears them down on every scroll tick and + // opens a race where files that scroll into view are never re-observed. + const nearViewportRef = useRef>(new Set()); + const [, bumpNearViewport] = useReducer((n: number) => n + 1, 0); + const nearViewportObserverRef = useRef(null); + const activeFileObserverRef = useRef(null); + const onActiveFileChangeRef = useRef(onActiveFileChange); + + useEffect(() => { + onActiveFileChangeRef.current = onActiveFileChange; + }, [onActiveFileChange]); + useEffect(() => { const panel = diffPanelRef.current; - if (!panel || visibleFiles.length === 0) return; + if (!panel) return; - const observer = new IntersectionObserver( + const nearObserver = new IntersectionObserver( (entries) => { + let changed = false; for (const entry of entries) { if (!entry.isIntersecting) continue; - const filename = entry.target.getAttribute("data-filename"); - if (filename) { - onActiveFileChange(filename); + if (filename && !nearViewportRef.current.has(filename)) { + nearViewportRef.current.add(filename); + nearObserver.unobserve(entry.target); + changed = true; } } + if (changed) bumpNearViewport(); }, { root: panel, - rootMargin: "-10% 0px -80% 0px", + rootMargin: "1500px 0px", threshold: 0, }, ); - for (const file of visibleFiles) { - const element = document.getElementById(encodeFileId(file.filename)); - if (element) observer.observe(element); - } - - return () => observer.disconnect(); - }, [onActiveFileChange, visibleFiles]); - - const [nearViewportFiles, setNearViewportFiles] = useState>( - () => new Set(), - ); - - useEffect(() => { - if (visibleFiles.length === 0 || nearViewportFiles.size > 0) return; - setNearViewportFiles( - new Set(visibleFiles.slice(0, 4).map((f) => f.filename)), - ); - }, [visibleFiles, nearViewportFiles.size]); - - useEffect(() => { - const panel = diffPanelRef.current; - if (!panel || visibleFiles.length === 0) return; - - const observer = new IntersectionObserver( + const activeObserver = new IntersectionObserver( (entries) => { - const newlyVisible: string[] = []; for (const entry of entries) { if (!entry.isIntersecting) continue; const filename = entry.target.getAttribute("data-filename"); - if (filename) { - newlyVisible.push(filename); - observer.unobserve(entry.target); - } - } - if (newlyVisible.length > 0) { - setNearViewportFiles((prev) => { - const next = new Set(prev); - for (const f of newlyVisible) next.add(f); - return next; - }); + if (filename) onActiveFileChangeRef.current(filename); } }, { root: panel, - rootMargin: "1500px 0px", + rootMargin: "-10% 0px -80% 0px", threshold: 0, }, ); + nearViewportObserverRef.current = nearObserver; + activeFileObserverRef.current = activeObserver; + + return () => { + nearObserver.disconnect(); + activeObserver.disconnect(); + nearViewportObserverRef.current = null; + activeFileObserverRef.current = null; + }; + }, []); + + useEffect(() => { + if (visibleFiles.length === 0 || nearViewportRef.current.size > 0) { + return; + } + for (const file of visibleFiles.slice(0, 4)) { + nearViewportRef.current.add(file.filename); + } + bumpNearViewport(); + }, [visibleFiles]); + + useEffect(() => { + const nearObserver = nearViewportObserverRef.current; + const activeObserver = activeFileObserverRef.current; + if (!nearObserver || !activeObserver) return; + for (const file of visibleFiles) { - if (nearViewportFiles.has(file.filename)) continue; const element = document.getElementById(encodeFileId(file.filename)); - if (element) observer.observe(element); + if (!element) continue; + activeObserver.observe(element); + if (!nearViewportRef.current.has(file.filename)) { + nearObserver.observe(element); + } } - - return () => observer.disconnect(); - }, [visibleFiles, nearViewportFiles]); + }, [visibleFiles]); if (files.length === 0) { return ( @@ -264,14 +281,6 @@ export const CommitDiffPane = memo( ); } - const noopStartComment = ( - _filename: string, - _range: SelectedLineRange, - ) => {}; - const noopCancel = () => {}; - const noopAdd = (_comment: PendingComment) => {}; - const noopEdit = (_original: PendingComment, _newBody: string) => {}; - return (
@@ -282,7 +291,7 @@ export const CommitDiffPane = memo( id={encodeFileId(file.filename)} file={file} diffStyle={diffStyle} - isNearViewport={nearViewportFiles.has(file.filename)} + isNearViewport={nearViewportRef.current.has(file.filename)} annotations={EMPTY_ANNOTATIONS} repliesByCommentId={EMPTY_REPLIES} owner="" diff --git a/apps/dashboard/src/lib/github.functions.ts b/apps/dashboard/src/lib/github.functions.ts index 4cebfba..6527c0c 100644 --- a/apps/dashboard/src/lib/github.functions.ts +++ b/apps/dashboard/src/lib/github.functions.ts @@ -30,6 +30,8 @@ import type { PullCommit, PullDetail, PullFile, + PullFileSummariesPage, + PullFileSummariesPageInput, PullFileSummary, PullFilesPage, PullFilesPageInput, @@ -67,8 +69,8 @@ import { type GitHubOrganization, isRepoVisibleWithInstallationAccess, } from "./github-access"; -import { shouldReauthorizeGitHubApp } from "./github-auth-errors"; import { getGitHubAppSlug } from "./github-app.server"; +import { shouldReauthorizeGitHubApp } from "./github-auth-errors"; import { bumpGitHubCacheNamespaces, bustGitHubCache, @@ -6083,50 +6085,165 @@ async function getPullFilesResult( }); } +type GraphQLPullFileSummariesResponse = { + repository: { + pullRequest: { + files: { + nodes: Array<{ + path: string; + changeType: + | "ADDED" + | "CHANGED" + | "COPIED" + | "DELETED" + | "MODIFIED" + | "RENAMED"; + additions: number; + deletions: number; + }>; + pageInfo: { hasNextPage: boolean; endCursor: string | null }; + }; + }; + }; +}; + +function mapGraphQLChangeTypeToStatus( + changeType: GraphQLPullFileSummariesResponse["repository"]["pullRequest"]["files"]["nodes"][number]["changeType"], +): PullFileSummary["status"] { + switch (changeType) { + case "ADDED": + return "added"; + case "DELETED": + return "removed"; + case "RENAMED": + return "renamed"; + case "COPIED": + return "copied"; + case "CHANGED": + return "changed"; + default: + return "modified"; + } +} + +const PULL_FILE_SUMMARIES_GRAPHQL_QUERY = `query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + files(first: 100, after: $cursor) { + nodes { path changeType additions deletions } + pageInfo { hasNextPage endCursor } + } + } + } +}`; + +// GitHub caps GraphQL `first:` at 100 for pullRequest.files, but we can +// loop several pages server-side and return them as one client-visible +// batch. 5 × 100 = 500 summaries per client round-trip cuts a 3k-file PR +// from ~30 client round-trips down to ~6. +const PULL_FILE_SUMMARIES_PAGES_PER_BATCH = 5; + +// File summaries power the sidebar tree and the "N files / +A / -D" chip. +// Returns one batch (up to ~500 files) so the client can render incrementally +// — otherwise a 3k-file PR blocks the sidebar for ~9s while all 30 pages +// load server-side. Each batch is cached independently by its starting +// cursor, so revisits hit the cache for every batch. async function getPullFileSummariesResult( context: GitHubContext, - data: PullFromRepoInput, -): Promise { + data: PullFileSummariesPageInput, +): Promise { const pullNamespaceKey = githubRevalidationSignalKeys.pullEntity({ owner: data.owner, repo: data.repo, pullNumber: data.pullNumber, }); - return getCachedPaginatedGitHubRequest({ - context, - resource: "pulls.fileSummaries", - params: data, + return getOrRevalidateGitHubResource({ + userId: context.session.user.id, + resource: "pulls.fileSummaries.graphql.batch", + params: { + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + cursor: data.cursor ?? null, + }, freshForMs: githubCachePolicy.detail.staleTimeMs, signalKeys: [pullNamespaceKey], namespaceKeys: [pullNamespaceKey], cacheMode: "split", - request: (page) => - context.octokit.rest.pulls.listFiles({ - owner: data.owner, - repo: data.repo, - pull_number: data.pullNumber, - page, - per_page: 100, - }), - mapData: (files) => - files.map((file) => ({ - filename: file.filename, - status: file.status as PullFileSummary["status"], - additions: file.additions, - deletions: file.deletions, - changes: file.changes, - previousFilename: file.previous_filename ?? null, - })), + fetcher: async () => { + const items: PullFileSummary[] = []; + let cursor: string | null = data.cursor ?? null; + let hasNextPage = true; + const deadlineAt = Date.now() + GITHUB_PAGINATED_OPERATION_TIMEOUT_MS; + + for (let i = 0; i < PULL_FILE_SUMMARIES_PAGES_PER_BATCH; i++) { + if (!hasNextPage) break; + + const timeoutMs = getRemainingSearchTimeoutMs( + deadlineAt, + GITHUB_OPERATION_TIMEOUT_MS, + ); + if (timeoutMs < GITHUB_MIN_OPERATION_TIMEOUT_MS) { + throw new GitHubOperationTimeoutError( + "github pulls.fileSummaries.graphql.batch", + timeoutMs, + ); + } + + const response = await withGitHubOperationTimeout( + "github pulls.fileSummaries.graphql.batch", + timeoutMs, + (signal) => + context.octokit.graphql( + PULL_FILE_SUMMARIES_GRAPHQL_QUERY, + { + owner: data.owner, + repo: data.repo, + number: data.pullNumber, + cursor, + request: { signal }, + }, + ), + ); + + const page = response.repository.pullRequest.files; + for (const node of page.nodes) { + items.push({ + filename: node.path, + status: mapGraphQLChangeTypeToStatus(node.changeType), + additions: node.additions, + deletions: node.deletions, + changes: node.additions + node.deletions, + // GraphQL's PullRequestFile doesn't expose the pre-rename path; + // the sidebar tree doesn't use it, and the diff view still gets + // previousFilename via the REST `getPullFiles` endpoint. + previousFilename: null, + }); + } + + hasNextPage = page.pageInfo.hasNextPage; + cursor = page.pageInfo.endCursor ?? null; + } + + return { + kind: "success", + data: { + items, + nextCursor: hasNextPage ? cursor : null, + }, + metadata: createGitHubResponseMetadata(200, {}), + }; + }, }); } export const getPullFileSummaries = createServerFn({ method: "GET" }) - .inputValidator(identityValidator) - .handler(async ({ data }): Promise => { + .inputValidator(identityValidator) + .handler(async ({ data }): Promise => { const context = await getGitHubContextForRepository(data); if (!context) { - return []; + return { items: [], nextCursor: null }; } return getPullFileSummariesResult(context, data); diff --git a/apps/dashboard/src/lib/github.query.ts b/apps/dashboard/src/lib/github.query.ts index f5df9b3..1b6466e 100644 --- a/apps/dashboard/src/lib/github.query.ts +++ b/apps/dashboard/src/lib/github.query.ts @@ -430,9 +430,12 @@ export function githubPullFileSummariesQueryOptions( scope: GitHubQueryScope, input: PullFromRepoQueryInput, ) { - return queryOptions({ + return infiniteQueryOptions({ queryKey: githubQueryKeys.pulls.fileSummaries(scope, input), - queryFn: () => getPullFileSummaries({ data: input }), + queryFn: ({ pageParam }) => + getPullFileSummaries({ data: { ...input, cursor: pageParam } }), + initialPageParam: null as string | null, + getNextPageParam: (lastPage) => lastPage.nextCursor ?? undefined, staleTime: githubCachePolicy.detail.staleTimeMs, gcTime: githubCachePolicy.detail.gcTimeMs, meta: tabPersistedMeta, diff --git a/apps/dashboard/src/lib/github.types.ts b/apps/dashboard/src/lib/github.types.ts index 5e50b3a..8f7e797 100644 --- a/apps/dashboard/src/lib/github.types.ts +++ b/apps/dashboard/src/lib/github.types.ts @@ -327,6 +327,18 @@ export type PullFile = { export type PullFileSummary = Omit; +export type PullFileSummariesPageInput = { + owner: string; + repo: string; + pullNumber: number; + cursor?: string | null; +}; + +export type PullFileSummariesPage = { + items: PullFileSummary[]; + nextCursor: string | null; +}; + export type PullFilesPageInput = { owner: string; repo: string; diff --git a/apps/dashboard/src/lib/query-client.tsx b/apps/dashboard/src/lib/query-client.tsx index dba757a..9c626f0 100644 --- a/apps/dashboard/src/lib/query-client.tsx +++ b/apps/dashboard/src/lib/query-client.tsx @@ -60,6 +60,18 @@ function isRepoQueryKeyInput( ); } +function isRepoCommitQueryKeyInput( + value: unknown, +): value is { owner: string; repo: string; sha: string } { + return Boolean( + value && + typeof value === "object" && + typeof (value as { owner?: unknown }).owner === "string" && + typeof (value as { repo?: unknown }).repo === "string" && + typeof (value as { sha?: unknown }).sha === "string", + ); +} + function matchesTabQuery(queryKey: readonly unknown[], tab: Tab) { const resourceType = queryKey[2]; const resourceName = queryKey[3]; @@ -80,6 +92,40 @@ function matchesTabQuery(queryKey: readonly unknown[], tab: Tab) { ); } + if (tab.type === "review") { + // Persist everything needed to rehydrate the review workspace instantly + // (toolbar PR header, sidebar tree, review threads, comments). Patches + // (`files`) are deliberately excluded — for a 3k-file PR they'd exceed + // localStorage quota and serializing them on every cache write would be + // expensive. Patches fall back to the server-side KV cache instead. + return ( + resourceType === "pulls" && + (resourceName === "page" || + resourceName === "fileSummaries" || + resourceName === "reviewComments" || + resourceName === "reviewThreads") && + isPullQueryKeyInput(input) && + input.owner === owner && + input.repo === repo && + input.pullNumber === tab.number + ); + } + + if (tab.type === "commit") { + if ( + resourceType !== "repo" || + resourceName !== "commit" || + !isRepoCommitQueryKeyInput(input) || + input.owner !== owner || + input.repo !== repo + ) { + return false; + } + // Commit tabs aren't keyed by a number — extract sha from the tab URL. + const shaMatch = tab.url.match(/\/commit\/([0-9a-f]+)/i); + return shaMatch ? input.sha === shaMatch[1] : false; + } + if (tab.type === "repo") { return ( resourceType === "repo" && diff --git a/apps/dashboard/src/routes/_protected/$owner/$repo/review.$pullId.tsx b/apps/dashboard/src/routes/_protected/$owner/$repo/review.$pullId.tsx index f6e8e8d..2936dbe 100644 --- a/apps/dashboard/src/routes/_protected/$owner/$repo/review.$pullId.tsx +++ b/apps/dashboard/src/routes/_protected/$owner/$repo/review.$pullId.tsx @@ -39,7 +39,7 @@ export const Route = createFileRoute("/_protected/$owner/$repo/review/$pullId")( // Never block navigation — fire prefetches and let the component // show cached data instantly or a skeleton while loading. void context.queryClient.prefetchQuery(pageOptions); - void context.queryClient.prefetchQuery(fileSummariesOptions); + void context.queryClient.prefetchInfiniteQuery(fileSummariesOptions); // Prefetch first page of files if not cached const filesQueryKey = githubQueryKeys.pulls.files(scope, input); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index bcfeb29..d6cf06e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -74,6 +74,9 @@ importers: '@tanstack/react-start': specifier: ~1.167.23 version: 1.167.23(react-dom@19.2.4(react@19.2.4))(react@19.2.4)(vite@7.3.2(@types/node@22.19.17)(jiti@2.6.1)(lightningcss@1.32.0)(tsx@4.21.0)(yaml@2.8.3)) + '@tanstack/react-virtual': + specifier: ^3.13.24 + version: 3.13.24(react-dom@19.2.4(react@19.2.4))(react@19.2.4) '@tanstack/router-plugin': specifier: ~1.167.12 version: 1.167.12(@tanstack/react-router@1.168.13(react-dom@19.2.4(react@19.2.4))(react@19.2.4))(vite@7.3.2(@types/node@22.19.17)(jiti@2.6.1)(lightningcss@1.32.0)(tsx@4.21.0)(yaml@2.8.3)) @@ -2817,6 +2820,12 @@ packages: react: ^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0 react-dom: ^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0 + '@tanstack/react-virtual@3.13.24': + resolution: {integrity: sha512-aIJvz5OSkhNIhZIpYivrxrPTKYsjW9Uzy+sP/mx0S3sev2HyvPb7xmjbYvokzEpfgYHy/HjzJ2zFAETuUfgCpg==} + peerDependencies: + react: ^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0 + react-dom: ^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0 + '@tanstack/router-core@1.168.9': resolution: {integrity: sha512-18oeEwEDyXOIuO1VBP9ACaK7tYHZUjynGDCoUh/5c/BNhia9vCJCp9O0LfhZXOorDc/PmLSgvmweFhVmIxF10g==} engines: {node: '>=20.19'} @@ -2901,6 +2910,9 @@ packages: '@tanstack/store@0.9.3': resolution: {integrity: sha512-8reSzl/qGWGGVKhBoxXPMWzATSbZLZFWhwBAFO9NAyp0TxzfBP0mIrGb8CP8KrQTmvzXlR/vFPPUrHTLBGyFyw==} + '@tanstack/virtual-core@3.14.0': + resolution: {integrity: sha512-JLANqGy/D6k4Ujmh8Tr25lGimuOXNiaVyXaCAZS0W+1390sADdGnyUdSWNIfd49gebtIxGMij4IktRVzrdr12Q==} + '@tanstack/virtual-file-routes@1.161.7': resolution: {integrity: sha512-olW33+Cn+bsCsZKPwEGhlkqS6w3M2slFv11JIobdnCFKMLG97oAI2kWKdx5/zsywTL8flpnoIgaZZPlQTFYhdQ==} engines: {node: '>=20.19'} @@ -7285,6 +7297,12 @@ snapshots: react-dom: 19.2.4(react@19.2.4) use-sync-external-store: 1.6.0(react@19.2.4) + '@tanstack/react-virtual@3.13.24(react-dom@19.2.4(react@19.2.4))(react@19.2.4)': + dependencies: + '@tanstack/virtual-core': 3.14.0 + react: 19.2.4 + react-dom: 19.2.4(react@19.2.4) + '@tanstack/router-core@1.168.9': dependencies: '@tanstack/history': 1.161.6 @@ -7422,6 +7440,8 @@ snapshots: '@tanstack/store@0.9.3': {} + '@tanstack/virtual-core@3.14.0': {} + '@tanstack/virtual-file-routes@1.161.7': {} '@testing-library/dom@10.4.1':