From 1eb6c6e84b48ce9eec75dcbcd197bb60a8e5371b Mon Sep 17 00:00:00 2001 From: Alan Daniel Date: Fri, 10 Apr 2026 00:09:01 -0400 Subject: [PATCH] Optimize review page with chunked loading, lazy diff rendering, and scroll persistence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Split SSR to only load PR metadata + file summaries, deferring patch payloads to client - Add paginated infinite query for patch files (50 per page) with scroll-based loading - Lazy-render diff viewers via shared IntersectionObserver — only mount PatchDiff when near viewport - Fall back from word diff to line diff for large patches (>400 changes or >24K chars) - Disable automatic refetch for page/files queries on the review route - Defer review comments loading until after mount - Update URL hash on scroll via replaceState for scroll persistence on reload - Fix deep-link navigation to files not yet loaded (fetches pages until target is found) - Fix scroll jank caused by hash handler re-firing on every file load - Polish: textarea bg-background, binary file card inset to match patch header --- .../pulls/review/review-diff-pane.tsx | 368 ++++++++++++++++++ .../pulls/review/review-file-diff-block.tsx | 46 ++- .../pulls/review/review-file-tree.tsx | 8 +- .../components/pulls/review/review-page.tsx | 305 +++++++++------ .../components/pulls/review/review-utils.ts | 9 +- apps/dashboard/src/lib/github.functions.ts | 147 ++++++- apps/dashboard/src/lib/github.query.ts | 17 + apps/dashboard/src/lib/github.types.ts | 15 + .../src/lib/use-github-revalidation.ts | 1 + .../$owner/$repo/review.$pullId.tsx | 24 +- packages/ui/src/components/textarea.tsx | 2 +- 11 files changed, 770 insertions(+), 172 deletions(-) create mode 100644 apps/dashboard/src/components/pulls/review/review-diff-pane.tsx diff --git a/apps/dashboard/src/components/pulls/review/review-diff-pane.tsx b/apps/dashboard/src/components/pulls/review/review-diff-pane.tsx new file mode 100644 index 0000000..afd66df --- /dev/null +++ b/apps/dashboard/src/components/pulls/review/review-diff-pane.tsx @@ -0,0 +1,368 @@ +import type { SelectedLineRange } from "@pierre/diffs"; +import type { DiffLineAnnotation } from "@pierre/diffs/react"; +import { + forwardRef, + memo, + useCallback, + useEffect, + useImperativeHandle, + useMemo, + useRef, + useState, +} from "react"; +import type { PullFile, PullReviewComment } from "#/lib/github.types"; +import { ReviewFileDiffBlock } from "./review-file-diff-block"; +import type { ActiveCommentForm, PendingComment } from "./review-types"; +import { encodeFileId } from "./review-utils"; + +const INITIAL_VISIBLE_COUNT = 12; +const LOAD_MORE_CHUNK = 12; +const SCROLL_TARGET_BUFFER = 6; +const EMPTY_ANNOTATIONS: DiffLineAnnotation[] = []; +const EMPTY_PENDING_COMMENTS: PendingComment[] = []; + +export type ReviewDiffPaneHandle = { + scrollToFile: (filename: string) => void; +}; + +type ReviewDiffPaneProps = { + files: PullFile[]; + totalFileCount: number; + diffStyle: "unified" | "split"; + annotationsByFile: ReadonlyMap< + string, + DiffLineAnnotation[] + >; + pendingCommentsByFile: ReadonlyMap; + hasNextPage: boolean; + isFetchingNextPage: boolean; + onLoadMore: () => void; + activeCommentForm: ActiveCommentForm | null; + selectedLines: SelectedLineRange | null; + onActiveFileChange: (filename: string) => void; + onStartComment: (filename: string, range: SelectedLineRange) => void; + onCancelComment: () => void; + onAddComment: (comment: PendingComment) => void; +}; + +export const ReviewDiffPane = memo( + forwardRef(function ReviewDiffPane( + { + files, + totalFileCount, + diffStyle, + annotationsByFile, + pendingCommentsByFile, + hasNextPage, + isFetchingNextPage, + onLoadMore, + activeCommentForm, + selectedLines, + onActiveFileChange, + onStartComment, + onCancelComment, + onAddComment, + }, + ref, + ) { + const diffPanelRef = useRef(null); + const loadMoreRef = useRef(null); + const [visibleCount, setVisibleCount] = useState(() => + Math.min(files.length, INITIAL_VISIBLE_COUNT), + ); + const [scrollTarget, setScrollTarget] = useState(null); + + useEffect(() => { + setVisibleCount((previous) => + Math.min( + files.length, + Math.max(files.length === 0 ? 0 : INITIAL_VISIBLE_COUNT, previous), + ), + ); + }, [files.length]); + + const revealFile = useCallback( + (filename: string) => { + const targetIndex = files.findIndex( + (file) => file.filename === filename, + ); + if (targetIndex === -1) { + setScrollTarget(filename); + if (hasNextPage && !isFetchingNextPage) { + onLoadMore(); + } + return; + } + + setScrollTarget(filename); + setVisibleCount((previous) => + Math.min( + files.length, + Math.max(previous, targetIndex + 1 + SCROLL_TARGET_BUFFER), + ), + ); + }, + [files, hasNextPage, isFetchingNextPage, onLoadMore], + ); + + const handleHash = useCallback(() => { + if (typeof window === "undefined") return; + + const hash = window.location.hash.slice(1); + if (!hash) return; + + // Set as scroll target — the scroll target effect handles finding + // the element, expanding visibleCount, and fetching more pages + setScrollTarget(hash); + }, []); + + useImperativeHandle( + ref, + () => ({ + scrollToFile: revealFile, + }), + [revealFile], + ); + + useEffect(() => { + handleHash(); + window.addEventListener("hashchange", handleHash); + + return () => window.removeEventListener("hashchange", handleHash); + }, [handleHash]); + + useEffect(() => { + if (!scrollTarget) return; + + // scrollTarget may be a filename or an already-encoded hash id + // Try to find the file in loaded data and expand visibleCount to include it + const targetIndex = files.findIndex( + (file) => + file.filename === scrollTarget || + encodeFileId(file.filename) === scrollTarget, + ); + + if (targetIndex !== -1) { + const needed = targetIndex + 1 + SCROLL_TARGET_BUFFER; + if (needed > visibleCount) { + setVisibleCount(Math.min(files.length, needed)); + return; // will re-run once visibleCount updates and element is in DOM + } + } + + const frameId = requestAnimationFrame(() => { + const encodedId = encodeFileId(scrollTarget); + const element = + document.getElementById(encodedId) ?? + document.getElementById(scrollTarget); + if (!element) { + if (hasNextPage && !isFetchingNextPage) { + onLoadMore(); + return; + } + + if (!hasNextPage) { + setScrollTarget(null); + } + return; + } + + element.scrollIntoView({ block: "start" }); + const filename = element.getAttribute("data-filename"); + if (filename) onActiveFileChange(filename); + setScrollTarget(null); + }); + return () => cancelAnimationFrame(frameId); + }, [ + files, + hasNextPage, + isFetchingNextPage, + onActiveFileChange, + onLoadMore, + scrollTarget, + visibleCount, + ]); + + useEffect(() => { + const panel = diffPanelRef.current; + const sentinel = loadMoreRef.current; + if ( + !panel || + !sentinel || + (visibleCount >= files.length && (!hasNextPage || isFetchingNextPage)) + ) { + return; + } + + const observer = new IntersectionObserver( + (entries) => { + if (!entries[0]?.isIntersecting) return; + + if (visibleCount < files.length) { + setVisibleCount((previous) => + Math.min(files.length, previous + LOAD_MORE_CHUNK), + ); + return; + } + + if (hasNextPage && !isFetchingNextPage) { + onLoadMore(); + } + }, + { + root: panel, + rootMargin: "3000px 0px", + threshold: 0, + }, + ); + + observer.observe(sentinel); + return () => observer.disconnect(); + }, [ + files.length, + hasNextPage, + isFetchingNextPage, + onLoadMore, + visibleCount, + ]); + + const visibleFiles = useMemo( + () => files.slice(0, visibleCount), + [files, visibleCount], + ); + + // Single shared observer for active file tracking + useEffect(() => { + const panel = diffPanelRef.current; + if (!panel || visibleFiles.length === 0) return; + + const observer = new IntersectionObserver( + (entries) => { + for (const entry of entries) { + if (!entry.isIntersecting) continue; + + const filename = entry.target.getAttribute("data-filename"); + if (filename) { + onActiveFileChange(filename); + const hash = `#${encodeFileId(filename)}`; + if (window.location.hash !== hash) { + history.replaceState(null, "", hash); + } + } + } + }, + { + root: panel, + rootMargin: "-10% 0px -80% 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(), + ); + + useEffect(() => { + const panel = diffPanelRef.current; + if (!panel || visibleFiles.length === 0) return; + + const observer = 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; + }); + } + }, + { + root: panel, + rootMargin: "1500px 0px", + threshold: 0, + }, + ); + + for (const file of visibleFiles) { + if (nearViewportFiles.has(file.filename)) continue; + const element = document.getElementById(encodeFileId(file.filename)); + if (element) observer.observe(element); + } + + return () => observer.disconnect(); + }, [visibleFiles, nearViewportFiles]); + + if (totalFileCount === 0 && !hasNextPage) { + return ( +
+ No files changed in this pull request. +
+ ); + } + + return ( +
+
+ {visibleFiles.map((file) => ( + + ))} + + {(visibleCount < files.length || hasNextPage) && ( + <> +
+
+ Loaded patch payload for {files.length} of {totalFileCount}{" "} + files + {isFetchingNextPage ? "..." : ""} +
+ + )} +
+
+ ); + }), +); + +ReviewDiffPane.displayName = "ReviewDiffPane"; 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 aba19ed..c447ff5 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 @@ -9,7 +9,9 @@ import { type ComponentType, type LazyExoticComponent, lazy, + memo, Suspense, + useCallback, useEffect, useMemo, useRef, @@ -22,10 +24,13 @@ import type { PendingComment, ReviewAnnotation, } from "./review-types"; -import { buildPatchString, encodeFileId } from "./review-utils"; +import { buildPatchString } from "./review-utils"; type ReviewPatchDiffComponent = ComponentType>; +const LARGE_PATCH_CHANGE_THRESHOLD = 400; +const LARGE_PATCH_CHAR_THRESHOLD = 24_000; + const PatchDiff: LazyExoticComponent = lazy(() => import.meta.env.SSR ? Promise.resolve({ @@ -43,9 +48,11 @@ if (!import.meta.env.SSR) { }); } -export function ReviewFileDiffBlock({ +export const ReviewFileDiffBlock = memo(function ReviewFileDiffBlock({ + id, file, diffStyle, + isNearViewport, annotations, pendingComments, activeCommentForm, @@ -54,19 +61,25 @@ export function ReviewFileDiffBlock({ onCancelComment, onAddComment, }: { + id: string; file: PullFile; diffStyle: "unified" | "split"; + isNearViewport: boolean; annotations: DiffLineAnnotation[]; pendingComments: PendingComment[]; activeCommentForm: ActiveCommentForm | null; selectedLines: SelectedLineRange | null; - onGutterClick: (range: SelectedLineRange) => void; + onGutterClick: (filename: string, range: SelectedLineRange) => void; onCancelComment: () => void; onAddComment: (comment: PendingComment) => void; }) { const [isCollapsed, setIsCollapsed] = useState(false); const { resolvedTheme } = useTheme(); const isDark = resolvedTheme === "dark"; + const handleGutterUtilityClick = useCallback( + (range: SelectedLineRange) => onGutterClick(file.filename, range), + [file.filename, onGutterClick], + ); const allAnnotations = useMemo(() => { const result: DiffLineAnnotation[] = [...annotations]; @@ -100,6 +113,9 @@ export function ReviewFileDiffBlock({ const mutedFg = isDark ? "oklch(0.705 0.015 286.067)" : "oklch(0.552 0.016 285.938)"; + const useWordDiff = + file.changes <= LARGE_PATCH_CHANGE_THRESHOLD && + (file.patch?.length ?? 0) <= LARGE_PATCH_CHAR_THRESHOLD; const diffOptions = useMemo( () => ({ @@ -108,13 +124,14 @@ export function ReviewFileDiffBlock({ dark: "vercel-dark" as const, light: "vercel-light" as const, }, - lineDiffType: "word" as const, + lineDiffType: useWordDiff ? ("word" as const) : ("none" as const), + maxLineDiffLength: useWordDiff ? 1_000 : 200, hunkSeparators: "line-info" as const, overflow: "scroll" as const, disableFileHeader: true, enableGutterUtility: true, enableLineSelection: true, - onGutterUtilityClick: onGutterClick, + onGutterUtilityClick: handleGutterUtilityClick, unsafeCSS: [ `:host { color-scheme: ${isDark ? "dark" : "light"}; ${isDark ? "" : "--diffs-light-bg: oklch(0.967 0.001 286.375);"} }`, `:host { --diffs-font-family: 'Geist Mono Variable', 'SF Mono', ui-monospace, 'Cascadia Code', monospace; }`, @@ -127,36 +144,37 @@ export function ReviewFileDiffBlock({ : `:host { --diffs-bg-addition-override: color-mix(in lab, var(--diffs-bg) 82%, var(--diffs-addition-base)); --diffs-bg-addition-number-override: color-mix(in lab, var(--diffs-bg) 78%, var(--diffs-addition-base)); --diffs-bg-deletion-override: color-mix(in lab, var(--diffs-bg) 82%, var(--diffs-deletion-base)); --diffs-bg-deletion-number-override: color-mix(in lab, var(--diffs-bg) 78%, var(--diffs-deletion-base)); }`, ].join("\n"), }), - [diffStyle, isDark, mutedFg, onGutterClick], + [diffStyle, handleGutterUtilityClick, isDark, mutedFg, useWordDiff], ); + const patchString = useMemo(() => buildPatchString(file), [file]); if (!file.patch) { return ( -
+
setIsCollapsed(!isCollapsed)} /> {!isCollapsed && ( -
- Binary file or diff too large to display +
+
+ Binary file or diff too large to display +
)}
); } - const patchString = buildPatchString(file); - return ( -
+
setIsCollapsed(!isCollapsed)} /> - {!isCollapsed && ( + {!isCollapsed && isNearViewport && (
); -} +}); function FileHeader({ file, 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 3e01e54..24ee48b 100644 --- a/apps/dashboard/src/components/pulls/review/review-file-tree.tsx +++ b/apps/dashboard/src/components/pulls/review/review-file-tree.tsx @@ -2,6 +2,7 @@ import { FileIcon, FolderIcon } from "@diffkit/icons"; import { cn } from "@diffkit/ui/lib/utils"; import { useState } from "react"; import type { FileTreeNode } from "./review-types"; +import { encodeFileId } from "./review-utils"; export function ReviewFileTreeNode({ node, @@ -61,10 +62,11 @@ export function ReviewFileTreeNode({ } const isActive = activeFile === node.path; + const fileId = encodeFileId(node.path); return ( - + ); } diff --git a/apps/dashboard/src/components/pulls/review/review-page.tsx b/apps/dashboard/src/components/pulls/review/review-page.tsx index 141aaea..e4f3380 100644 --- a/apps/dashboard/src/components/pulls/review/review-page.tsx +++ b/apps/dashboard/src/components/pulls/review/review-page.tsx @@ -7,13 +7,26 @@ import { import { cn } from "@diffkit/ui/lib/utils"; import type { SelectedLineRange } from "@pierre/diffs"; import type { DiffLineAnnotation } from "@pierre/diffs/react"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { + useInfiniteQuery, + useQuery, + useQueryClient, +} from "@tanstack/react-query"; import { getRouteApi, Link } from "@tanstack/react-router"; -import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { + lazy, + Suspense, + useCallback, + useDeferredValue, + useEffect, + useMemo, + useRef, + useState, +} from "react"; import { getPrStateConfig } from "#/components/pulls/detail/pull-detail-header"; -import { submitPullReview } from "#/lib/github.functions"; +import { getPullFiles, submitPullReview } from "#/lib/github.functions"; import { - githubPullFilesQueryOptions, + githubPullFileSummariesQueryOptions, githubPullPageQueryOptions, githubPullReviewCommentsQueryOptions, githubQueryKeys, @@ -21,7 +34,7 @@ import { import type { PullReviewComment } from "#/lib/github.types"; import { useHasMounted } from "#/lib/use-has-mounted"; import { useRegisterTab } from "#/lib/use-register-tab"; -import { ReviewFileDiffBlock } from "./review-file-diff-block"; +import type { ReviewDiffPaneHandle } from "./review-diff-pane"; import { ReviewFileTreeNode } from "./review-file-tree"; import { ReviewSubmitPopover } from "./review-submit-popover"; import type { @@ -30,37 +43,73 @@ import type { PendingComment, ReviewEvent, } from "./review-types"; -import { buildFileTree, encodeFileId } from "./review-utils"; +import { buildFileTree } from "./review-utils"; const routeApi = getRouteApi("/_protected/$owner/$repo/review/$pullId"); +const PULL_FILES_PAGE_SIZE = 50; +const ReviewDiffPane = lazy(() => + import("./review-diff-pane").then((mod) => ({ + default: mod.ReviewDiffPane, + })), +); export function ReviewPage() { const { user } = routeApi.useRouteContext(); + const loaderData = routeApi.useLoaderData(); const { owner, repo, pullId } = routeApi.useParams(); const pullNumber = Number(pullId); const scope = { userId: user.id }; const hasMounted = useHasMounted(); const queryClient = useQueryClient(); const input = { owner, repo, pullNumber }; + const diffPaneRef = useRef(null); + const [shouldLoadReviewComments, setShouldLoadReviewComments] = + useState(false); const pageQuery = useQuery({ ...githubPullPageQueryOptions(scope, input), - enabled: hasMounted, + refetchOnMount: false, + refetchOnWindowFocus: false, }); - const filesQuery = useQuery({ - ...githubPullFilesQueryOptions(scope, input), + const fileSummariesQuery = useQuery({ + ...githubPullFileSummariesQueryOptions(scope, input), + refetchOnMount: false, + refetchOnWindowFocus: false, + }); + + const filesQuery = useInfiniteQuery({ + queryKey: githubQueryKeys.pulls.files(scope, input), + initialPageParam: 1, enabled: hasMounted, + queryFn: ({ pageParam }) => + getPullFiles({ + data: { + ...input, + page: pageParam, + perPage: PULL_FILES_PAGE_SIZE, + }, + }), + getNextPageParam: (lastPage) => lastPage.nextPage ?? undefined, + refetchOnMount: false, + refetchOnWindowFocus: false, }); const reviewCommentsQuery = useQuery({ ...githubPullReviewCommentsQueryOptions(scope, input), - enabled: hasMounted, + enabled: shouldLoadReviewComments, + refetchOnWindowFocus: false, }); - const pr = pageQuery.data?.detail; - const files = filesQuery.data ?? []; + const pr = pageQuery.data?.detail ?? loaderData?.pageData?.detail ?? null; + const sidebarFiles = + fileSummariesQuery.data ?? loaderData?.fileSummaries ?? []; + const diffFiles = useMemo( + () => filesQuery.data?.pages.flatMap((page) => page.files) ?? [], + [filesQuery.data], + ); const reviewComments = reviewCommentsQuery.data ?? []; + const hasDiffPayload = filesQuery.data !== undefined; const [diffStyle, setDiffStyle] = useState<"unified" | "split">("unified"); const [pendingComments, setPendingComments] = useState([]); @@ -72,8 +121,7 @@ export function ReviewPage() { const [activeFile, setActiveFile] = useState(null); const [fileFilter, setFileFilter] = useState(""); const [isSubmitting, setIsSubmitting] = useState(false); - - const diffPanelRef = useRef(null); + const deferredFileFilter = useDeferredValue(fileFilter); useRegisterTab( pr @@ -90,11 +138,11 @@ export function ReviewPage() { : null, ); - const fileTree = useMemo(() => buildFileTree(files), [files]); + const fileTree = useMemo(() => buildFileTree(sidebarFiles), [sidebarFiles]); const filteredTree = useMemo(() => { - if (!fileFilter) return fileTree; - const lower = fileFilter.toLowerCase(); + if (!deferredFileFilter) return fileTree; + const lower = deferredFileFilter.toLowerCase(); function filterNodes(nodes: FileTreeNode[]): FileTreeNode[] { return nodes @@ -112,44 +160,13 @@ export function ReviewPage() { } return filterNodes(fileTree); - }, [fileFilter, fileTree]); + }, [deferredFileFilter, fileTree]); const scrollToFile = useCallback((filename: string) => { - const element = document.getElementById(encodeFileId(filename)); - if (element) { - element.scrollIntoView({ behavior: "smooth", block: "start" }); - setActiveFile(filename); - } + diffPaneRef.current?.scrollToFile(filename); + setActiveFile(filename); }, []); - useEffect(() => { - const panel = diffPanelRef.current; - if (!panel || files.length === 0) return; - - const observer = new IntersectionObserver( - (entries) => { - for (const entry of entries) { - if (entry.isIntersecting) { - const filename = entry.target.getAttribute("data-filename"); - if (filename) setActiveFile(filename); - } - } - }, - { - root: panel, - rootMargin: "-10% 0px -80% 0px", - threshold: 0, - }, - ); - - for (const file of files) { - const element = document.getElementById(encodeFileId(file.filename)); - if (element) observer.observe(element); - } - - return () => observer.disconnect(); - }, [files]); - const annotationsByFile = useMemo(() => { const map = new Map[]>(); for (const comment of reviewComments) { @@ -165,11 +182,83 @@ export function ReviewPage() { return map; }, [reviewComments]); + const pendingCommentsByFile = useMemo(() => { + const map = new Map(); + for (const comment of pendingComments) { + const existing = map.get(comment.path) ?? []; + existing.push(comment); + map.set(comment.path, existing); + } + 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]); + + useEffect(() => { + if (!hasMounted || !hasDiffPayload || shouldLoadReviewComments) return; + + const timeoutId = window.setTimeout(() => { + setShouldLoadReviewComments(true); + }, 250); + + return () => window.clearTimeout(timeoutId); + }, [hasDiffPayload, hasMounted, shouldLoadReviewComments]); + const addPendingComment = useCallback((comment: PendingComment) => { setPendingComments((previous) => [...previous, comment]); setActiveCommentForm(null); }, []); + const handleCancelComment = useCallback(() => { + setActiveCommentForm(null); + setSelectedLines(null); + }, []); + + const handleAddComment = useCallback( + (comment: PendingComment) => { + addPendingComment(comment); + setSelectedLines(null); + }, + [addPendingComment], + ); + + const handleStartComment = useCallback( + (filename: string, range: SelectedLineRange) => { + const isMultiLine = range.start !== range.end; + const startIsSmaller = range.start <= range.end; + const lineSide = startIsSmaller + ? (range.endSide ?? range.side) + : range.side; + const startLineSide = startIsSmaller + ? range.side + : (range.endSide ?? range.side); + const toGithubSide = (side: string | undefined) => + side === "deletions" ? ("LEFT" as const) : ("RIGHT" as const); + + setActiveCommentForm({ + path: filename, + line: Math.max(range.start, range.end), + side: toGithubSide(lineSide), + ...(isMultiLine + ? { + startLine: Math.min(range.start, range.end), + startSide: toGithubSide(startLineSide), + } + : {}), + }); + setSelectedLines(range); + }, + [], + ); + const handleSubmitReview = useCallback( async (body: string, event: ReviewEvent) => { setIsSubmitting(true); @@ -211,6 +300,9 @@ export function ReviewPage() { ); if (pageQuery.error) throw pageQuery.error; + if (fileSummariesQuery.error) throw fileSummariesQuery.error; + if (filesQuery.error) throw filesQuery.error; + if (reviewCommentsQuery.error) throw reviewCommentsQuery.error; if (!pr) { return ( @@ -222,8 +314,7 @@ export function ReviewPage() { const stateConfig = getPrStateConfig(pr); const StateIcon = stateConfig.icon; - const totalAdditions = files.reduce((sum, file) => sum + file.additions, 0); - const totalDeletions = files.reduce((sum, file) => sum + file.deletions, 0); + const sidebarFileCount = sidebarFiles.length; return (
@@ -251,15 +342,15 @@ export function ReviewPage() { - {files.length} + {sidebarFileCount} {" "} - {files.length === 1 ? "file" : "files"} + {sidebarFileCount === 1 ? "file" : "files"} - +{totalAdditions} + +{diffStats.totalAdditions} - -{totalDeletions} + -{diffStats.totalDeletions}
@@ -335,7 +426,8 @@ export function ReviewPage() {
- {files.length} {files.length === 1 ? "file" : "files"} changed + {sidebarFileCount} {sidebarFileCount === 1 ? "file" : "files"}{" "} + changed
@@ -343,73 +435,42 @@ export function ReviewPage() { -
-
- {files.map((file) => ( - comment.path === file.filename, - )} - activeCommentForm={ - activeCommentForm?.path === file.filename - ? activeCommentForm - : null - } - selectedLines={ - activeCommentForm?.path === file.filename - ? selectedLines - : null + {hasMounted && hasDiffPayload ? ( + }> + { + if ( + filesQuery.hasNextPage && + !filesQuery.isFetchingNextPage + ) { + void filesQuery.fetchNextPage(); } - onGutterClick={(range) => { - const isMultiLine = range.start !== range.end; - const startIsSmaller = range.start <= range.end; - const lineSide = startIsSmaller - ? (range.endSide ?? range.side) - : range.side; - const startLineSide = startIsSmaller - ? range.side - : (range.endSide ?? range.side); - const toGithubSide = (s: string | undefined) => - s === "deletions" - ? ("LEFT" as const) - : ("RIGHT" as const); - setActiveCommentForm({ - path: file.filename, - line: Math.max(range.start, range.end), - side: toGithubSide(lineSide), - ...(isMultiLine - ? { - startLine: Math.min(range.start, range.end), - startSide: toGithubSide(startLineSide), - } - : {}), - }); - setSelectedLines(range); - }} - onCancelComment={() => { - setActiveCommentForm(null); - setSelectedLines(null); - }} - onAddComment={(comment) => { - addPendingComment(comment); - setSelectedLines(null); - }} - /> - ))} - - {files.length === 0 && !filesQuery.isLoading && ( -
- No files changed in this pull request. -
- )} -
-
+ }} + activeCommentForm={activeCommentForm} + selectedLines={selectedLines} + onActiveFileChange={setActiveFile} + onStartComment={handleStartComment} + onCancelComment={handleCancelComment} + onAddComment={handleAddComment} + /> + + ) : ( + + )}
); } + +function ReviewDiffPanePlaceholder() { + return
; +} diff --git a/apps/dashboard/src/components/pulls/review/review-utils.ts b/apps/dashboard/src/components/pulls/review/review-utils.ts index f03db50..5c9415d 100644 --- a/apps/dashboard/src/components/pulls/review/review-utils.ts +++ b/apps/dashboard/src/components/pulls/review/review-utils.ts @@ -1,7 +1,12 @@ -import type { PullFile } from "#/lib/github.types"; +import type { PullFile, PullFileSummary } from "#/lib/github.types"; import type { FileTreeNode } from "./review-types"; -export function buildFileTree(files: PullFile[]): FileTreeNode[] { +type FileTreeEntry = Pick< + PullFileSummary, + "filename" | "status" | "additions" | "deletions" +>; + +export function buildFileTree(files: FileTreeEntry[]): FileTreeNode[] { const root: FileTreeNode = { name: "", path: "", diff --git a/apps/dashboard/src/lib/github.functions.ts b/apps/dashboard/src/lib/github.functions.ts index 4d4120a..b85ee6e 100644 --- a/apps/dashboard/src/lib/github.functions.ts +++ b/apps/dashboard/src/lib/github.functions.ts @@ -16,6 +16,9 @@ import type { PullCommit, PullDetail, PullFile, + PullFileSummary, + PullFilesPage, + PullFilesPageInput, PullPageData, PullReviewComment, PullStatus, @@ -668,6 +671,62 @@ async function getCachedGitHubRequest({ }); } +async function getCachedPaginatedGitHubRequest({ + context, + resource, + params, + freshForMs, + signalKeys, + request, + mapData, + pageSize = 100, +}: { + context: GitHubContext; + resource: string; + params: unknown; + freshForMs: number; + signalKeys?: string[]; + request: (page: number) => Promise>; + mapData: (items: TGitHubItem[]) => TResult; + pageSize?: number; +}) { + return getOrRevalidateGitHubResource({ + userId: context.session.user.id, + resource, + params, + freshForMs, + signalKeys, + fetcher: async () => { + const items: TGitHubItem[] = []; + let page = 1; + let metadata = createGitHubResponseMetadata(200, {}); + + while (true) { + const response = await request(page); + if (page === 1) { + metadata = createGitHubResponseMetadata( + response.status, + normalizeResponseHeaders(response.headers), + ); + } + + items.push(...response.data); + if (response.data.length < pageSize) { + break; + } + + page += 1; + } + + return { + kind: "success", + data: mapData(items), + metadata, + }; + }, + }); +} + async function getCachedPullResponse({ context, data, @@ -1784,12 +1843,21 @@ export const mergePullRequest = createServerFn({ method: "POST" }) async function getPullFilesResult( context: GitHubContext, - data: PullFromRepoInput, -): Promise { - return getCachedGitHubRequest({ + data: PullFilesPageInput, +): Promise { + const page = clampPage(data.page); + const perPage = clampPerPage(data.perPage, 20); + + return getCachedGitHubRequest({ context, - resource: "pulls.files", - params: data, + resource: "pulls.filesPage", + params: { + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + page, + perPage, + }, freshForMs: githubCachePolicy.detail.staleTimeMs, signalKeys: [ githubRevalidationSignalKeys.pullEntity({ @@ -1803,11 +1871,12 @@ async function getPullFilesResult( owner: data.owner, repo: data.repo, pull_number: data.pullNumber, - per_page: 300, + page, + per_page: perPage, headers, }), - mapData: (files) => - files.map((file) => ({ + mapData: (files) => ({ + files: files.map((file) => ({ sha: file.sha, filename: file.filename, status: file.status as PullFile["status"], @@ -1817,17 +1886,66 @@ async function getPullFilesResult( patch: file.patch ?? null, previousFilename: file.previous_filename ?? null, })), + nextPage: files.length === perPage ? page + 1 : null, + }), }); } -export const getPullFiles = createServerFn({ method: "GET" }) +async function getPullFileSummariesResult( + context: GitHubContext, + data: PullFromRepoInput, +): Promise { + return getCachedPaginatedGitHubRequest({ + context, + resource: "pulls.fileSummaries", + params: data, + freshForMs: githubCachePolicy.detail.staleTimeMs, + signalKeys: [ + githubRevalidationSignalKeys.pullEntity({ + owner: data.owner, + repo: data.repo, + pullNumber: data.pullNumber, + }), + ], + 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, + })), + }); +} + +export const getPullFileSummaries = createServerFn({ method: "GET" }) .inputValidator(identityValidator) - .handler(async ({ data }): Promise => { + .handler(async ({ data }): Promise => { const context = await getGitHubContext(); if (!context) { return []; } + return getPullFileSummariesResult(context, data); + }); + +export const getPullFiles = createServerFn({ method: "GET" }) + .inputValidator(identityValidator) + .handler(async ({ data }): Promise => { + const context = await getGitHubContext(); + if (!context) { + return { files: [], nextPage: null }; + } + return getPullFilesResult(context, data); }); @@ -1835,7 +1953,10 @@ async function getPullReviewCommentsResult( context: GitHubContext, data: PullFromRepoInput, ): Promise { - return getCachedGitHubRequest({ + return getCachedPaginatedGitHubRequest< + RepoPullReviewComment, + PullReviewComment[] + >({ context, resource: "pulls.reviewComments", params: data, @@ -1847,13 +1968,13 @@ async function getPullReviewCommentsResult( pullNumber: data.pullNumber, }), ], - request: (headers) => + request: (page) => context.octokit.rest.pulls.listReviewComments({ owner: data.owner, repo: data.repo, pull_number: data.pullNumber, + page, per_page: 100, - headers, }), mapData: (comments) => comments.map((comment) => ({ diff --git a/apps/dashboard/src/lib/github.query.ts b/apps/dashboard/src/lib/github.query.ts index eb68a19..1c541df 100644 --- a/apps/dashboard/src/lib/github.query.ts +++ b/apps/dashboard/src/lib/github.query.ts @@ -10,6 +10,7 @@ import { getMyPulls, getOrgTeams, getPullComments, + getPullFileSummaries, getPullFiles, getPullFromRepo, getPullPageData, @@ -118,6 +119,8 @@ export const githubQueryKeys = { ["github", scope.userId, "pulls", "page", input] as const, detail: (scope: GitHubQueryScope, input: PullFromRepoQueryInput) => ["github", scope.userId, "pulls", "detail", input] as const, + fileSummaries: (scope: GitHubQueryScope, input: PullFromRepoQueryInput) => + ["github", scope.userId, "pulls", "fileSummaries", input] as const, comments: (scope: GitHubQueryScope, input: PullFromRepoQueryInput) => ["github", scope.userId, "pulls", "comments", input] as const, status: (scope: GitHubQueryScope, input: PullFromRepoQueryInput) => @@ -280,6 +283,20 @@ export function githubPullFilesQueryOptions( }); } +export function githubPullFileSummariesQueryOptions( + scope: GitHubQueryScope, + input: PullFromRepoQueryInput, +) { + return queryOptions({ + queryKey: githubQueryKeys.pulls.fileSummaries(scope, input), + queryFn: () => getPullFileSummaries({ data: input }), + staleTime: githubCachePolicy.detail.staleTimeMs, + gcTime: githubCachePolicy.detail.gcTimeMs, + refetchOnMount: "always", + meta: tabPersistedMeta, + }); +} + export function githubPullReviewCommentsQueryOptions( scope: GitHubQueryScope, input: PullFromRepoQueryInput, diff --git a/apps/dashboard/src/lib/github.types.ts b/apps/dashboard/src/lib/github.types.ts index 9aa01ba..b878af3 100644 --- a/apps/dashboard/src/lib/github.types.ts +++ b/apps/dashboard/src/lib/github.types.ts @@ -197,6 +197,21 @@ export type PullFile = { previousFilename: string | null; }; +export type PullFileSummary = Omit; + +export type PullFilesPageInput = { + owner: string; + repo: string; + pullNumber: number; + page?: number; + perPage?: number; +}; + +export type PullFilesPage = { + files: PullFile[]; + nextPage: number | null; +}; + export type PullReviewComment = { id: number; body: string; diff --git a/apps/dashboard/src/lib/use-github-revalidation.ts b/apps/dashboard/src/lib/use-github-revalidation.ts index 8ce9d4d..06cf9f4 100644 --- a/apps/dashboard/src/lib/use-github-revalidation.ts +++ b/apps/dashboard/src/lib/use-github-revalidation.ts @@ -38,6 +38,7 @@ async function invalidatePullTabQueries( const queryKeys = [ githubQueryKeys.pulls.page(scope, input), githubQueryKeys.pulls.detail(scope, input), + githubQueryKeys.pulls.fileSummaries(scope, input), githubQueryKeys.pulls.comments(scope, input), githubQueryKeys.pulls.status(scope, input), githubQueryKeys.pulls.files(scope, input), 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 d5b07c6..bbebe6a 100644 --- a/apps/dashboard/src/routes/_protected/$owner/$repo/review.$pullId.tsx +++ b/apps/dashboard/src/routes/_protected/$owner/$repo/review.$pullId.tsx @@ -1,9 +1,8 @@ import { createFileRoute } from "@tanstack/react-router"; import { ReviewPage } from "#/components/pulls/review/review-page"; import { - githubPullFilesQueryOptions, + githubPullFileSummariesQueryOptions, githubPullPageQueryOptions, - githubPullReviewCommentsQueryOptions, } from "#/lib/github.query"; import { buildSeo, formatPageTitle, summarizeText } from "#/lib/seo"; @@ -14,8 +13,7 @@ export const Route = createFileRoute("/_protected/$owner/$repo/review/$pullId")( const scope = { userId: context.user.id }; const input = { owner: params.owner, repo: params.repo, pullNumber }; const pageOptions = githubPullPageQueryOptions(scope, input); - const filesOptions = githubPullFilesQueryOptions(scope, input); - const commentsOptions = githubPullReviewCommentsQueryOptions( + const fileSummariesOptions = githubPullFileSummariesQueryOptions( scope, input, ); @@ -24,22 +22,14 @@ export const Route = createFileRoute("/_protected/$owner/$repo/review/$pullId")( context.queryClient.getQueryData(pageOptions.queryKey) ?? (await context.queryClient.ensureQueryData(pageOptions)); - if ( - context.queryClient.getQueryData(filesOptions.queryKey) === undefined - ) { - await context.queryClient.ensureQueryData(filesOptions); - } + const fileSummaries = + context.queryClient.getQueryData(fileSummariesOptions.queryKey) ?? + (await context.queryClient.ensureQueryData(fileSummariesOptions)); - if ( - context.queryClient.getQueryData(commentsOptions.queryKey) === undefined - ) { - await context.queryClient.ensureQueryData(commentsOptions); - } - - return pageData; + return { pageData, fileSummaries }; }, head: ({ loaderData, match, params }) => { - const pull = loaderData?.detail; + const pull = loaderData?.pageData?.detail; const title = pull ? formatPageTitle(`Review PR #${pull.number}: ${pull.title}`) : formatPageTitle(`Review PR #${params.pullId}`); diff --git a/packages/ui/src/components/textarea.tsx b/packages/ui/src/components/textarea.tsx index 9a99ec0..d601086 100644 --- a/packages/ui/src/components/textarea.tsx +++ b/packages/ui/src/components/textarea.tsx @@ -7,7 +7,7 @@ function Textarea({ className, ...props }: React.ComponentProps<"textarea">) {