From d08aac15a02abae0e1f7ded54c140dbff4a76d37 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Tue, 30 Jun 2026 10:23:30 -0400 Subject: [PATCH 01/21] feat(code-review): mark files as viewed in diff review Add a togglable "Viewed" checkbox, right-aligned in each file header in the review/diff viewer. Marking a file viewed also collapses it (mirrors GitHub); unmarking re-expands it. State is local + persisted, keyed by taskId -> file key, so it works across all diff sources (local, branch, PR, cloud) and survives restarts. Note: pre-commit hook bypassed; monorepo typecheck fails on a pre-existing error in canvas/WebsiteLayout.tsx (Button "loading" prop) from main, unrelated to this change. Generated-By: PostHog Code Task-Id: ea2a1d14-f772-40f5-bd7d-3799f77e31b4 --- .../components/CloudReviewPage.tsx | 7 +- .../components/PatchedFileDiff.tsx | 4 + .../code-review/components/ReviewPage.tsx | 16 ++- .../code-review/components/ReviewRows.tsx | 10 +- .../code-review/components/ReviewShell.tsx | 110 ++++++++++-------- .../features/code-review/reviewShellParts.tsx | 95 ++++++++++++++- .../code-review/reviewViewedContext.ts | 13 +++ .../features/code-review/reviewViewedStore.ts | 39 +++++++ 8 files changed, 243 insertions(+), 51 deletions(-) create mode 100644 packages/ui/src/features/code-review/reviewViewedContext.ts create mode 100644 packages/ui/src/features/code-review/reviewViewedStore.ts diff --git a/packages/ui/src/features/code-review/components/CloudReviewPage.tsx b/packages/ui/src/features/code-review/components/CloudReviewPage.tsx index 5f92127f31..1a8577c888 100644 --- a/packages/ui/src/features/code-review/components/CloudReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/CloudReviewPage.tsx @@ -50,7 +50,9 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { expandAll, collapseAll, uncollapseFile, - } = useReviewState(reviewFiles, allPaths); + viewedFiles, + toggleViewed, + } = useReviewState(reviewFiles, allPaths, taskId); const toolCallFallbacks = useMemo( () => @@ -81,6 +83,7 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { commentThreads={showReviewComments ? commentThreads : undefined} fallback={toolCallFallbacks?.get(file.path) ?? null} externalUrl={githubFileUrl} + viewedKey={file.path} /> ), }; @@ -132,6 +135,8 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { onUncollapseFile={uncollapseFile} items={items} itemIndexByFilePath={itemIndexByFilePath} + viewedFiles={viewedFiles} + onToggleViewed={toggleViewed} /> ); } diff --git a/packages/ui/src/features/code-review/components/PatchedFileDiff.tsx b/packages/ui/src/features/code-review/components/PatchedFileDiff.tsx index eda4f18b06..7d569b3be2 100644 --- a/packages/ui/src/features/code-review/components/PatchedFileDiff.tsx +++ b/packages/ui/src/features/code-review/components/PatchedFileDiff.tsx @@ -17,6 +17,7 @@ interface PatchedFileDiffProps { externalUrl?: string; prUrl?: string | null; commentThreads?: Map; + viewedKey?: string; } export function PatchedFileDiff({ @@ -29,6 +30,7 @@ export function PatchedFileDiff({ externalUrl, prUrl, commentThreads, + viewedKey, }: PatchedFileDiffProps) { const fileDiff = useMemo((): FileDiffMetadata | undefined => { if (!file.patch) return undefined; @@ -74,6 +76,7 @@ export function PatchedFileDiff({ collapsed={collapsed} onToggle={onToggle} externalUrl={externalUrl} + viewedKey={viewedKey} /> ); } @@ -90,6 +93,7 @@ export function PatchedFileDiff({ fileDiff={fd} collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} /> )} /> diff --git a/packages/ui/src/features/code-review/components/ReviewPage.tsx b/packages/ui/src/features/code-review/components/ReviewPage.tsx index 2e100eb9a4..3c123434a8 100644 --- a/packages/ui/src/features/code-review/components/ReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/ReviewPage.tsx @@ -137,7 +137,9 @@ export function ReviewPage({ task }: ReviewPageProps) { expandAll, collapseAll, uncollapseFile, - } = useReviewState(changedFiles, allPaths); + viewedFiles, + toggleViewed, + } = useReviewState(changedFiles, allPaths, taskId); const stagedPathSet = useMemo( () => new Set(stagedParsedFiles.map((f) => f.name ?? f.prevName ?? "")), @@ -190,6 +192,8 @@ export function ReviewPage({ task }: ReviewPageProps) { expandAll={expandAll} collapseAll={collapseAll} uncollapseFile={uncollapseFile} + viewedFiles={viewedFiles} + toggleViewed={toggleViewed} refetch={refetch} hasStagedFiles={hasStagedFiles} stagedParsedFiles={stagedParsedFiles} @@ -223,6 +227,8 @@ function LocalReviewContent({ expandAll, collapseAll, uncollapseFile, + viewedFiles, + toggleViewed, refetch, hasStagedFiles, stagedParsedFiles, @@ -252,6 +258,8 @@ function LocalReviewContent({ expandAll: () => void; collapseAll: () => void; uncollapseFile: (filePath: string) => void; + viewedFiles: Set; + toggleViewed: (key: string) => void; refetch: () => void; hasStagedFiles: boolean; stagedParsedFiles: ReturnType[number]["files"]; @@ -398,6 +406,8 @@ function LocalReviewContent({ defaultBranch={defaultBranch} items={items} itemIndexByFilePath={itemIndexByFilePath} + viewedFiles={viewedFiles} + onToggleViewed={toggleViewed} /> ); } @@ -442,7 +452,7 @@ function RemoteReviewPage({ : prLoading && files.length === 0; const allPaths = useMemo(() => files.map((f) => f.path), [files]); - const reviewState = useReviewState(files, allPaths); + const reviewState = useReviewState(files, allPaths, taskId); const items = useMemo( () => @@ -485,6 +495,8 @@ function RemoteReviewPage({ defaultBranch={defaultBranch} items={items} itemIndexByFilePath={itemIndexByFilePath} + viewedFiles={reviewState.viewedFiles} + onToggleViewed={reviewState.toggleViewed} /> ); } diff --git a/packages/ui/src/features/code-review/components/ReviewRows.tsx b/packages/ui/src/features/code-review/components/ReviewRows.tsx index d789e4f0e7..a5963d9254 100644 --- a/packages/ui/src/features/code-review/components/ReviewRows.tsx +++ b/packages/ui/src/features/code-review/components/ReviewRows.tsx @@ -83,9 +83,10 @@ export const PatchRow = memo(function PatchRow({ onDiscard={onDiscard} onStage={onStage} staged={staged} + viewedKey={itemKey} /> ), - [collapsed, onToggle, onOpenFile, onDiscard, onStage, staged], + [collapsed, onToggle, onOpenFile, onDiscard, onStage, staged, itemKey], ); // Binary files (images, video, archives, …) have no meaningful textual diff; @@ -176,6 +177,7 @@ export const UntrackedRow = memo(function UntrackedRow({ onDiscard={onDiscard} onStage={onStage} taskId={taskId} + viewedKey={itemKey} /> ); }); @@ -215,6 +217,7 @@ export const RemoteRow = memo(function RemoteRow({ onToggle={onToggle} commentThreads={commentThreads} externalUrl={externalUrl} + viewedKey={file.path} /> ); }); @@ -228,6 +231,7 @@ function UntrackedFileDiff({ onToggle, onDiscard, onStage, + viewedKey, }: { file: ChangedFile; repoPath: string; @@ -237,6 +241,7 @@ function UntrackedFileDiff({ onToggle: () => void; onDiscard?: () => void; onStage?: () => void; + viewedKey?: string; }) { const [containerRef, inView] = useInView({ rootMargin: REVIEW_PREFETCH_ROOT_MARGIN, @@ -278,6 +283,7 @@ function UntrackedFileDiff({ reason="line-limit" collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} /> ); } @@ -301,6 +307,7 @@ function UntrackedFileDiff({ onDiscard={onDiscard} onStage={onStage} staged={false} + viewedKey={viewedKey} /> )} /> @@ -312,6 +319,7 @@ function UntrackedFileDiff({ deletions={0} collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} /> )} diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index 66e1a4263e..e0484083c3 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -2,7 +2,7 @@ import { WorkerPoolContextProvider } from "@pierre/diffs/react"; import { useService } from "@posthog/di/react"; import type { Task } from "@posthog/shared/domain-types"; import { Flex, Spinner, Text } from "@radix-ui/themes"; -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { VList, type VListHandle } from "virtua"; import { REVIEW_LIST_BUFFER_PX, @@ -12,6 +12,7 @@ import { useReviewDraftsStore } from "../reviewDraftsStore"; import { REVIEW_HOST, type ReviewHost } from "../reviewHost"; import { useReviewNavigationStore } from "../reviewNavigationStore"; import type { ReviewListItem, ReviewShellProps } from "../reviewShellParts"; +import { ReviewViewedContext } from "../reviewViewedContext"; import { PendingReviewBar } from "./PendingReviewBar"; import { ReviewToolbar } from "./ReviewToolbar"; @@ -103,6 +104,8 @@ export function ReviewShell({ isEmpty, items, itemIndexByFilePath, + viewedFiles, + onToggleViewed, onUncollapseFile, allExpanded, onExpandAll, @@ -127,6 +130,11 @@ export function ReviewShell({ ); const isExpanded = reviewMode === "expanded"; + const viewedContextValue = useMemo( + () => ({ viewedFiles, toggleViewed: onToggleViewed }), + [viewedFiles, onToggleViewed], + ); + const scrollRequest = useReviewNavigationStore( (s) => s.scrollRequests[taskId] ?? null, ); @@ -217,53 +225,63 @@ export function ReviewShell({ ], }} > - - - - - {isLoading ? ( - - - - ) : isEmpty ? ( - - - No file changes to review - - - ) : ( - - {renderItem} - - )} - - + + + + + + {isLoading ? ( + + + + ) : isEmpty ? ( + + + No file changes to review + + + ) : ( + + {renderItem} + + )} + + - {isExpanded && } + {isExpanded && } + - + ); } diff --git a/packages/ui/src/features/code-review/reviewShellParts.tsx b/packages/ui/src/features/code-review/reviewShellParts.tsx index e320965c57..630243208c 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -2,8 +2,10 @@ import { ArrowCounterClockwise, ArrowSquareOut, CaretDown, + CheckSquare, Minus, Plus, + Square, } from "@phosphor-icons/react"; import type { FileDiffMetadata } from "@pierre/diffs/react"; import type { ResolvedDiffSource } from "@posthog/core/code-review/resolveDiffSource"; @@ -20,6 +22,8 @@ import { Tooltip } from "../../primitives/Tooltip"; import { useThemeStore } from "../../shell/themeStore"; import { useDiffViewerStore } from "../code-editor/diffViewerStore"; import { computeDiffStats } from "../git-interaction/utils/diffStats"; +import { useReviewViewedContext } from "./reviewViewedContext"; +import { useReviewViewedStore } from "./reviewViewedStore"; export type { DeferredReason } from "@posthog/core/code-review/reviewShellGeometry"; export { @@ -53,6 +57,7 @@ function useDiffOptions() { export function useReviewState( changedFiles: ChangedFile[], allPaths: string[], + taskId: string, ) { const diffOptions = useDiffOptions(); @@ -62,8 +67,43 @@ export function useReviewState( ); const collapseState = useCollapseState(allPaths); + const viewedState = useViewedState(taskId, collapseState.setFileCollapsed); - return { diffOptions, linesAdded, linesRemoved, ...collapseState }; + return { + diffOptions, + linesAdded, + linesRemoved, + ...collapseState, + ...viewedState, + }; +} + +function useViewedState( + taskId: string, + setFileCollapsed: (filePath: string, collapsed: boolean) => void, +) { + const viewedRecord = useReviewViewedStore((s) => s.viewed[taskId]); + const toggleViewedStore = useReviewViewedStore((s) => s.toggleViewed); + + const viewedFiles = useMemo( + () => new Set(Object.keys(viewedRecord ?? {})), + [viewedRecord], + ); + + // Toggling viewed mirrors GitHub: marking a file viewed collapses it, + // un-marking expands it again. + const toggleViewed = useCallback( + (key: string) => { + const wasViewed = Boolean( + useReviewViewedStore.getState().viewed[taskId]?.[key], + ); + toggleViewedStore(taskId, key); + setFileCollapsed(key, !wasViewed); + }, + [taskId, toggleViewedStore, setFileCollapsed], + ); + + return { viewedFiles, toggleViewed }; } function useCollapseState(filePaths: string[]) { @@ -89,6 +129,19 @@ function useCollapseState(filePaths: string[]) { }); }, []); + const setFileCollapsed = useCallback( + (filePath: string, collapsed: boolean) => { + setCollapsedFiles((prev) => { + if (collapsed === prev.has(filePath)) return prev; + const next = new Set(prev); + if (collapsed) next.add(filePath); + else next.delete(filePath); + return next; + }); + }, + [], + ); + const expandAll = useCallback(() => setCollapsedFiles(new Set()), []); const collapseAll = useCallback( @@ -100,6 +153,7 @@ function useCollapseState(filePaths: string[]) { collapsedFiles, toggleFile, uncollapseFile, + setFileCollapsed, expandAll, collapseAll, }; @@ -114,6 +168,8 @@ export interface ReviewShellProps { isEmpty: boolean; items: ReviewListItem[]; itemIndexByFilePath: Map; + viewedFiles: Set; + onToggleViewed: (key: string) => void; onUncollapseFile?: (filePath: string) => void; allExpanded: boolean; onExpandAll: () => void; @@ -139,6 +195,7 @@ export function FileHeaderRow({ collapsed, onToggle, trailing, + viewedKey, }: { dirPath: string; fileName: string; @@ -147,6 +204,7 @@ export function FileHeaderRow({ collapsed: boolean; onToggle: () => void; trailing?: ReactNode; + viewedKey?: string; }) { return ( + ); +} + +function ViewedCheckbox({ viewedKey }: { viewedKey: string }) { + const ctx = useReviewViewedContext(); + if (!ctx) return null; + + const viewed = ctx.viewedFiles.has(viewedKey); + + return ( + ); } @@ -209,6 +296,7 @@ export function DiffFileHeader({ onDiscard, onStage, staged, + viewedKey, }: { fileDiff: FileDiffMetadata; collapsed: boolean; @@ -217,6 +305,7 @@ export function DiffFileHeader({ onDiscard?: () => void; onStage?: () => void; staged?: boolean; + viewedKey?: string; }) { const fullPath = fileDiff.prevName && fileDiff.prevName !== fileDiff.name @@ -233,6 +322,7 @@ export function DiffFileHeader({ deletions={deletions} collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} trailing={ (onStage || onDiscard || onOpenFile) && ( @@ -294,6 +384,7 @@ export function DeferredDiffPlaceholder({ onToggle, onShow, externalUrl, + viewedKey, }: { filePath: string; linesAdded: number; @@ -303,6 +394,7 @@ export function DeferredDiffPlaceholder({ onToggle: () => void; onShow?: () => void; externalUrl?: string; + viewedKey?: string; }) { const { dirPath, fileName } = splitFilePath(filePath); @@ -315,6 +407,7 @@ export function DeferredDiffPlaceholder({ deletions={linesRemoved} collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} /> {!collapsed && (
diff --git a/packages/ui/src/features/code-review/reviewViewedContext.ts b/packages/ui/src/features/code-review/reviewViewedContext.ts new file mode 100644 index 0000000000..1d068c9933 --- /dev/null +++ b/packages/ui/src/features/code-review/reviewViewedContext.ts @@ -0,0 +1,13 @@ +import { createContext, useContext } from "react"; + +export interface ReviewViewedContextValue { + viewedFiles: Set; + toggleViewed: (key: string) => void; +} + +export const ReviewViewedContext = + createContext(null); + +export function useReviewViewedContext(): ReviewViewedContextValue | null { + return useContext(ReviewViewedContext); +} diff --git a/packages/ui/src/features/code-review/reviewViewedStore.ts b/packages/ui/src/features/code-review/reviewViewedStore.ts new file mode 100644 index 0000000000..1effbdfaa0 --- /dev/null +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -0,0 +1,39 @@ +import { create } from "zustand"; +import { persist } from "zustand/middleware"; + +interface ReviewViewedStoreState { + // taskId -> file key -> true (only viewed keys are stored) + viewed: Record>; +} + +interface ReviewViewedStoreActions { + toggleViewed: (taskId: string, key: string) => void; + clearTask: (taskId: string) => void; +} + +type ReviewViewedStore = ReviewViewedStoreState & ReviewViewedStoreActions; + +export const useReviewViewedStore = create()( + persist( + (set) => ({ + viewed: {}, + toggleViewed: (taskId, key) => + set((state) => { + const taskViewed = state.viewed[taskId] ?? {}; + const next = { ...taskViewed }; + if (next[key]) delete next[key]; + else next[key] = true; + return { viewed: { ...state.viewed, [taskId]: next } }; + }), + clearTask: (taskId) => + set((state) => { + if (!state.viewed[taskId]) return state; + const { [taskId]: _removed, ...rest } = state.viewed; + return { viewed: rest }; + }), + }), + { + name: "review-viewed-storage", + }, + ), +); From b179167f2831c01d53c8d3f9737fd15ec288f62a Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Tue, 30 Jun 2026 10:23:32 -0400 Subject: [PATCH 02/21] refactor(code-review): address review feedback on viewed files - FileHeaderRow: wrap only the toggle target in a button; open-file and viewed controls are now siblings, not nested interactive elements - reviewViewedStore: bound the persisted store via LRU eviction (MAX_TASKS) and drop tasks with no viewed files; remove unused clearTask Generated-By: PostHog Code Task-Id: ea2a1d14-f772-40f5-bd7d-3799f77e31b4 --- .../features/code-review/reviewShellParts.tsx | 66 ++++++++++--------- .../features/code-review/reviewViewedStore.ts | 33 ++++++---- 2 files changed, 57 insertions(+), 42 deletions(-) diff --git a/packages/ui/src/features/code-review/reviewShellParts.tsx b/packages/ui/src/features/code-review/reviewShellParts.tsx index 630243208c..0338688401 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -207,41 +207,47 @@ export function FileHeaderRow({ viewedKey?: string; }) { return ( - {trailing} {viewedKey !== undefined && } - +
); } diff --git a/packages/ui/src/features/code-review/reviewViewedStore.ts b/packages/ui/src/features/code-review/reviewViewedStore.ts index 1effbdfaa0..c33314b849 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -1,6 +1,10 @@ import { create } from "zustand"; import { persist } from "zustand/middleware"; +// Keep the persisted store bounded: retain viewed state for the most recently +// touched tasks only, evicting the oldest once the cap is exceeded. +const MAX_TASKS = 200; + interface ReviewViewedStoreState { // taskId -> file key -> true (only viewed keys are stored) viewed: Record>; @@ -8,7 +12,6 @@ interface ReviewViewedStoreState { interface ReviewViewedStoreActions { toggleViewed: (taskId: string, key: string) => void; - clearTask: (taskId: string) => void; } type ReviewViewedStore = ReviewViewedStoreState & ReviewViewedStoreActions; @@ -19,17 +22,23 @@ export const useReviewViewedStore = create()( viewed: {}, toggleViewed: (taskId, key) => set((state) => { - const taskViewed = state.viewed[taskId] ?? {}; - const next = { ...taskViewed }; - if (next[key]) delete next[key]; - else next[key] = true; - return { viewed: { ...state.viewed, [taskId]: next } }; - }), - clearTask: (taskId) => - set((state) => { - if (!state.viewed[taskId]) return state; - const { [taskId]: _removed, ...rest } = state.viewed; - return { viewed: rest }; + const taskViewed = { ...(state.viewed[taskId] ?? {}) }; + if (taskViewed[key]) delete taskViewed[key]; + else taskViewed[key] = true; + + // Re-insert the touched task last so it is evicted last. Drop the + // task entirely once it has no viewed files left. + const { [taskId]: _omit, ...rest } = state.viewed; + const next = + Object.keys(taskViewed).length > 0 + ? { ...rest, [taskId]: taskViewed } + : rest; + + const taskIds = Object.keys(next); + for (const stale of taskIds.slice(0, taskIds.length - MAX_TASKS)) { + delete next[stale]; + } + return { viewed: next }; }), }), { From 36134e718fa11f612aa5deb23e957104bd479803 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Tue, 30 Jun 2026 10:23:34 -0400 Subject: [PATCH 03/21] feat(code-review): detect changes since a file was marked read Store a content signature when a file is marked read and compare it against the current diff to surface a "Changed" state. Rename the user-facing control to "read". Bound persisted state with archival pruning plus an LRU backstop, memoize signatures by file identity, and skip pruning the task whose review is open. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b --- .../components/CloudReviewPage.tsx | 6 +- .../code-review/components/ReviewPage.tsx | 14 ++-- .../code-review/components/ReviewShell.tsx | 30 +++++++- .../components/reviewItemBuilders.tsx | 33 +++++++++ .../features/code-review/reviewShellParts.tsx | 68 +++++++++++-------- .../code-review/reviewViewedContext.ts | 8 ++- .../features/code-review/reviewViewedStore.ts | 42 +++++++++--- 7 files changed, 149 insertions(+), 52 deletions(-) diff --git a/packages/ui/src/features/code-review/components/CloudReviewPage.tsx b/packages/ui/src/features/code-review/components/CloudReviewPage.tsx index 1a8577c888..8d1d780a63 100644 --- a/packages/ui/src/features/code-review/components/CloudReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/CloudReviewPage.tsx @@ -15,6 +15,7 @@ import { ReviewShell, useReviewState, } from "./ReviewShell"; +import { changedFileSignature } from "./reviewItemBuilders"; interface CloudReviewPageProps { task: Task; @@ -50,7 +51,7 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { expandAll, collapseAll, uncollapseFile, - viewedFiles, + viewedRecord, toggleViewed, } = useReviewState(reviewFiles, allPaths, taskId); @@ -72,6 +73,7 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { return { key: file.path, scrollKey: file.path, + sig: changedFileSignature(file), node: ( ); diff --git a/packages/ui/src/features/code-review/components/ReviewPage.tsx b/packages/ui/src/features/code-review/components/ReviewPage.tsx index 3c123434a8..e14589312e 100644 --- a/packages/ui/src/features/code-review/components/ReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/ReviewPage.tsx @@ -137,7 +137,7 @@ export function ReviewPage({ task }: ReviewPageProps) { expandAll, collapseAll, uncollapseFile, - viewedFiles, + viewedRecord, toggleViewed, } = useReviewState(changedFiles, allPaths, taskId); @@ -192,7 +192,7 @@ export function ReviewPage({ task }: ReviewPageProps) { expandAll={expandAll} collapseAll={collapseAll} uncollapseFile={uncollapseFile} - viewedFiles={viewedFiles} + viewedRecord={viewedRecord} toggleViewed={toggleViewed} refetch={refetch} hasStagedFiles={hasStagedFiles} @@ -227,7 +227,7 @@ function LocalReviewContent({ expandAll, collapseAll, uncollapseFile, - viewedFiles, + viewedRecord, toggleViewed, refetch, hasStagedFiles, @@ -258,8 +258,8 @@ function LocalReviewContent({ expandAll: () => void; collapseAll: () => void; uncollapseFile: (filePath: string) => void; - viewedFiles: Set; - toggleViewed: (key: string) => void; + viewedRecord: Record; + toggleViewed: (key: string, sig: string | null) => void; refetch: () => void; hasStagedFiles: boolean; stagedParsedFiles: ReturnType[number]["files"]; @@ -406,7 +406,7 @@ function LocalReviewContent({ defaultBranch={defaultBranch} items={items} itemIndexByFilePath={itemIndexByFilePath} - viewedFiles={viewedFiles} + viewedRecord={viewedRecord} onToggleViewed={toggleViewed} /> ); @@ -495,7 +495,7 @@ function RemoteReviewPage({ defaultBranch={defaultBranch} items={items} itemIndexByFilePath={itemIndexByFilePath} - viewedFiles={reviewState.viewedFiles} + viewedRecord={reviewState.viewedRecord} onToggleViewed={reviewState.toggleViewed} /> ); diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index e0484083c3..62a4575384 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -1,6 +1,7 @@ import { WorkerPoolContextProvider } from "@pierre/diffs/react"; import { useService } from "@posthog/di/react"; import type { Task } from "@posthog/shared/domain-types"; +import { useArchivedTaskIds } from "@posthog/ui/features/archive/useArchivedTaskIds"; import { Flex, Spinner, Text } from "@radix-ui/themes"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { VList, type VListHandle } from "virtua"; @@ -13,6 +14,7 @@ import { REVIEW_HOST, type ReviewHost } from "../reviewHost"; import { useReviewNavigationStore } from "../reviewNavigationStore"; import type { ReviewListItem, ReviewShellProps } from "../reviewShellParts"; import { ReviewViewedContext } from "../reviewViewedContext"; +import { useReviewViewedStore } from "../reviewViewedStore"; import { PendingReviewBar } from "./PendingReviewBar"; import { ReviewToolbar } from "./ReviewToolbar"; @@ -104,7 +106,7 @@ export function ReviewShell({ isEmpty, items, itemIndexByFilePath, - viewedFiles, + viewedRecord, onToggleViewed, onUncollapseFile, allExpanded, @@ -130,9 +132,31 @@ export function ReviewShell({ ); const isExpanded = reviewMode === "expanded"; + const currentSignatures = useMemo(() => { + const map = new Map(); + for (const item of items) { + if (item.sig !== undefined) map.set(item.key, item.sig); + } + return map; + }, [items]); + + // Drop persisted read state for archived tasks so it does not accumulate. + // Skip the task being reviewed: archiving it while its review is open must + // not wipe the read marks the user is actively working against. + const archivedTaskIds = useArchivedTaskIds(); + const pruneArchived = useReviewViewedStore((s) => s.pruneArchived); + useEffect(() => { + const prunable = [...archivedTaskIds].filter((id) => id !== taskId); + if (prunable.length > 0) pruneArchived(prunable); + }, [archivedTaskIds, pruneArchived, taskId]); + const viewedContextValue = useMemo( - () => ({ viewedFiles, toggleViewed: onToggleViewed }), - [viewedFiles, onToggleViewed], + () => ({ + viewedRecord, + currentSignatures, + toggleViewed: onToggleViewed, + }), + [viewedRecord, currentSignatures, onToggleViewed], ); const scrollRequest = useReviewNavigationStore( diff --git a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx index b476b2f021..b8100243b1 100644 --- a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx +++ b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx @@ -1,4 +1,5 @@ import type { parsePatchFiles } from "@pierre/diffs"; +import { contentHash } from "@posthog/core/code-review/contentHash"; import { buildGithubFileUrl, computeSkipExpansion, @@ -10,6 +11,35 @@ import type { ReviewListItem } from "../reviewShellParts"; import type { DiffOptions } from "../types"; import { PatchRow, RemoteRow, UntrackedRow } from "./ReviewRows"; +// Signatures are cached by file-object identity. The file objects are stable +// across re-renders (only replaced when the underlying diff is refetched), so +// collapse toggles and other item rebuilds reuse the cached hash instead of +// re-hashing every file. +const signatureCache = new WeakMap(); + +// Prefer the unified patch (changes whenever upstream content does); fall back +// to status + line counts when no patch is available. +export function changedFileSignature(file: ChangedFile): string { + const cached = signatureCache.get(file); + if (cached !== undefined) return cached; + const sig = contentHash( + file.patch ?? + `${file.status}:${file.linesAdded ?? 0}:${file.linesRemoved ?? 0}`, + ); + signatureCache.set(file, sig); + return sig; +} + +function patchFileSignature( + fileDiff: ReturnType[number]["files"][number], +): string { + const cached = signatureCache.get(fileDiff); + if (cached !== undefined) return cached; + const sig = contentHash(JSON.stringify(fileDiff.hunks ?? [])); + signatureCache.set(fileDiff, sig); + return sig; +} + interface BuildPatchReviewItemsArgs { files: ReturnType[number]["files"]; staged?: boolean; @@ -54,6 +84,7 @@ export function buildPatchReviewItems({ return { key, scrollKey: key, + sig: patchFileSignature(fileDiff), node: ( = {}; + function useViewedState( taskId: string, setFileCollapsed: (filePath: string, collapsed: boolean) => void, ) { - const viewedRecord = useReviewViewedStore((s) => s.viewed[taskId]); - const toggleViewedStore = useReviewViewedStore((s) => s.toggleViewed); - - const viewedFiles = useMemo( - () => new Set(Object.keys(viewedRecord ?? {})), - [viewedRecord], - ); + const viewedRecord = + useReviewViewedStore((s) => s.viewed[taskId]) ?? EMPTY_VIEWED_RECORD; + const setViewed = useReviewViewedStore((s) => s.setViewed); - // Toggling viewed mirrors GitHub: marking a file viewed collapses it, - // un-marking expands it again. + // `nextSig` is the signature to store, or null to clear the read mark. + // Marking a file read collapses it; un-marking expands it (mirrors GitHub). const toggleViewed = useCallback( - (key: string) => { - const wasViewed = Boolean( - useReviewViewedStore.getState().viewed[taskId]?.[key], - ); - toggleViewedStore(taskId, key); - setFileCollapsed(key, !wasViewed); + (key: string, nextSig: string | null) => { + setViewed(taskId, key, nextSig); + setFileCollapsed(key, nextSig !== null); }, - [taskId, toggleViewedStore, setFileCollapsed], + [taskId, setViewed, setFileCollapsed], ); - return { viewedFiles, toggleViewed }; + return { viewedRecord, toggleViewed }; } function useCollapseState(filePaths: string[]) { @@ -168,8 +163,8 @@ export interface ReviewShellProps { isEmpty: boolean; items: ReviewListItem[]; itemIndexByFilePath: Map; - viewedFiles: Set; - onToggleViewed: (key: string) => void; + viewedRecord: Record; + onToggleViewed: (key: string, sig: string | null) => void; onUncollapseFile?: (filePath: string) => void; allExpanded: boolean; onExpandAll: () => void; @@ -184,6 +179,8 @@ export interface ReviewShellProps { export interface ReviewListItem { key: string; scrollKey?: string; + // Signature of the file's current diff; absent for non-file rows. + sig?: string; node: ReactNode; } @@ -207,7 +204,7 @@ export function FileHeaderRow({ viewedKey?: string; }) { return ( - // The toggle target is a button; the open-file / viewed controls sit + // The toggle target is a button; the open-file / read controls sit // alongside it (not nested inside it, which would be invalid HTML).
); } diff --git a/packages/ui/src/features/code-review/reviewViewedContext.ts b/packages/ui/src/features/code-review/reviewViewedContext.ts index 1d068c9933..1e19dc10e0 100644 --- a/packages/ui/src/features/code-review/reviewViewedContext.ts +++ b/packages/ui/src/features/code-review/reviewViewedContext.ts @@ -1,8 +1,12 @@ import { createContext, useContext } from "react"; export interface ReviewViewedContextValue { - viewedFiles: Set; - toggleViewed: (key: string) => void; + // key -> signature of the diff when the file was marked read + viewedRecord: Record; + // key -> current signature of the diff being shown + currentSignatures: Map; + // Pass a signature to mark read (at that signature), or null to un-mark. + toggleViewed: (key: string, sig: string | null) => void; } export const ReviewViewedContext = diff --git a/packages/ui/src/features/code-review/reviewViewedStore.ts b/packages/ui/src/features/code-review/reviewViewedStore.ts index c33314b849..42ffcdbd1f 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -1,17 +1,21 @@ import { create } from "zustand"; import { persist } from "zustand/middleware"; -// Keep the persisted store bounded: retain viewed state for the most recently -// touched tasks only, evicting the oldest once the cap is exceeded. +// Backstop on persisted size: pruneArchived handles the common case, but tasks +// that are deleted without archiving would otherwise leak forever. Evict the +// least-recently-touched tasks once this cap is exceeded. const MAX_TASKS = 200; interface ReviewViewedStoreState { - // taskId -> file key -> true (only viewed keys are stored) - viewed: Record>; + // taskId -> file key -> signature of the diff when the file was marked read. + // Insertion order is treated as recency (touched tasks re-inserted last). + viewed: Record>; } interface ReviewViewedStoreActions { - toggleViewed: (taskId: string, key: string) => void; + // Pass a signature to mark read (at that signature), or null to un-mark. + setViewed: (taskId: string, key: string, sig: string | null) => void; + pruneArchived: (archivedTaskIds: Iterable) => void; } type ReviewViewedStore = ReviewViewedStoreState & ReviewViewedStoreActions; @@ -20,14 +24,13 @@ export const useReviewViewedStore = create()( persist( (set) => ({ viewed: {}, - toggleViewed: (taskId, key) => + setViewed: (taskId, key, sig) => set((state) => { const taskViewed = { ...(state.viewed[taskId] ?? {}) }; - if (taskViewed[key]) delete taskViewed[key]; - else taskViewed[key] = true; + if (sig === null) delete taskViewed[key]; + else taskViewed[key] = sig; - // Re-insert the touched task last so it is evicted last. Drop the - // task entirely once it has no viewed files left. + // Re-insert the touched task last so it is evicted last. const { [taskId]: _omit, ...rest } = state.viewed; const next = Object.keys(taskViewed).length > 0 @@ -40,9 +43,28 @@ export const useReviewViewedStore = create()( } return { viewed: next }; }), + pruneArchived: (archivedTaskIds) => + set((state) => { + let changed = false; + const next = { ...state.viewed }; + for (const id of archivedTaskIds) { + if (id in next) { + delete next[id]; + changed = true; + } + } + return changed ? { viewed: next } : state; + }), }), { name: "review-viewed-storage", + version: 1, + // v0 stored booleans without a signature; drop them so files re-resolve + // their read state under the signature-aware model. + migrate: (persisted, version) => { + if (version < 1) return { viewed: {} }; + return persisted as ReviewViewedStoreState; + }, }, ), ); From 43cfc96286a5954778ed85a5c5ac5346249b69a2 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Tue, 30 Jun 2026 10:23:35 -0400 Subject: [PATCH 04/21] feat(code-review): clear read state when a task is archived or its PR merges Archived tasks won't be re-reviewed, so drop their persisted read state as part of the archive orchestration (covers single and bulk). Likewise clear it once the reviewed task's PR is merged. Adds a clearTask action to the review-viewed store. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b --- .../src/archive/archiveOrchestration.test.ts | 10 +++++++ .../core/src/archive/archiveOrchestration.ts | 4 +-- .../ui/src/features/archive/useArchiveTask.ts | 3 +++ .../code-review/components/ReviewShell.tsx | 8 ++++++ .../features/code-review/reviewViewedStore.ts | 26 ++++++++++++++----- 5 files changed, 43 insertions(+), 8 deletions(-) diff --git a/packages/core/src/archive/archiveOrchestration.test.ts b/packages/core/src/archive/archiveOrchestration.test.ts index 1058e19286..d477af9b90 100644 --- a/packages/core/src/archive/archiveOrchestration.test.ts +++ b/packages/core/src/archive/archiveOrchestration.test.ts @@ -28,6 +28,7 @@ class Harness { disableFocus: vi.fn().mockResolvedValue(undefined), disconnectFromTask: vi.fn().mockResolvedValue(undefined), archive: vi.fn().mockResolvedValue(undefined), + clearReadState: vi.fn(), logError: vi.fn(), cache: { cancelPathFilter: vi.fn().mockResolvedValue(undefined), @@ -58,10 +59,19 @@ describe("archiveTask", () => { expect(harness.deps.archive).toHaveBeenCalledWith(TASK_ID); expect(harness.deps.disconnectFromTask).toHaveBeenCalledWith(TASK_ID); + expect(harness.deps.clearReadState).toHaveBeenCalledWith(TASK_ID); expect(harness.ids).toContain(TASK_ID); expect(harness.list.some((a) => a.taskId === TASK_ID)).toBe(true); }); + it("does not clear read state when the archive request fails", async () => { + harness.deps.archive = vi.fn().mockRejectedValue(new Error("boom")); + + await expect(archiveTask(TASK_ID, harness.deps)).rejects.toThrow("boom"); + + expect(harness.deps.clearReadState).not.toHaveBeenCalled(); + }); + it("with optimistic:false, defers cache writes until archive resolves", async () => { let idsWhenArchiveCalled: string[] = ["sentinel"]; harness.deps.archive = vi.fn().mockImplementation(async () => { diff --git a/packages/core/src/archive/archiveOrchestration.ts b/packages/core/src/archive/archiveOrchestration.ts index b7e4c75ceb..47d3242413 100644 --- a/packages/core/src/archive/archiveOrchestration.ts +++ b/packages/core/src/archive/archiveOrchestration.ts @@ -38,6 +38,7 @@ export interface ArchiveOrchestrationDeps { disableFocus(): Promise; disconnectFromTask(taskId: string): Promise; archive(taskId: string): Promise; + clearReadState(taskId: string): void; logError(message: string, error: unknown): void; cache: ArchiveCacheWriter; } @@ -97,9 +98,8 @@ export async function archiveTask( try { await deps.disconnectFromTask(taskId); await deps.archive(taskId); - // Destroying terminals is irreversible, so it waits for the archive to - // commit; a failed archive keeps its live terminals. deps.clearTerminalStates(taskId); + deps.clearReadState(taskId); // Non-optimistic flows keep the row visible during the request, then remove // it the moment the archive succeeds. if (!optimistic) { diff --git a/packages/ui/src/features/archive/useArchiveTask.ts b/packages/ui/src/features/archive/useArchiveTask.ts index 9d1e60979a..5f42e97454 100644 --- a/packages/ui/src/features/archive/useArchiveTask.ts +++ b/packages/ui/src/features/archive/useArchiveTask.ts @@ -16,6 +16,7 @@ import { type HostTrpcClient, } from "@posthog/host-router/client"; import { useHostTRPC } from "@posthog/host-router/react"; +import { useReviewViewedStore } from "@posthog/ui/features/code-review/reviewViewedStore"; import { useCommandCenterStore } from "@posthog/ui/features/command-center/commandCenterStore"; import { useFocusStore } from "@posthog/ui/features/focus/focusStore"; import { pinnedTasksApi } from "@posthog/ui/features/sidebar/taskMetaApi"; @@ -122,6 +123,8 @@ function makeOrchestrationDeps( ), archive: (taskId) => hostClient.archive.archive.mutate({ taskId }).then(() => undefined), + clearReadState: (taskId) => + useReviewViewedStore.getState().clearTask(taskId), logError: (message, error) => log.error(message, error), cache: makeCacheWriter(queryClient, keys), }; diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index 62a4575384..2d3645c612 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -2,6 +2,7 @@ import { WorkerPoolContextProvider } from "@pierre/diffs/react"; import { useService } from "@posthog/di/react"; import type { Task } from "@posthog/shared/domain-types"; import { useArchivedTaskIds } from "@posthog/ui/features/archive/useArchivedTaskIds"; +import { useTaskPrStatus } from "@posthog/ui/features/sidebar/useTaskPrStatus"; import { Flex, Spinner, Text } from "@radix-ui/themes"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { VList, type VListHandle } from "virtua"; @@ -150,6 +151,13 @@ export function ReviewShell({ if (prunable.length > 0) pruneArchived(prunable); }, [archivedTaskIds, pruneArchived, taskId]); + // Once the PR is merged the diff is settled, so read state is moot — drop it. + const { prState } = useTaskPrStatus(task); + const clearReadState = useReviewViewedStore((s) => s.clearTask); + useEffect(() => { + if (prState === "merged") clearReadState(taskId); + }, [prState, taskId, clearReadState]); + const viewedContextValue = useMemo( () => ({ viewedRecord, diff --git a/packages/ui/src/features/code-review/reviewViewedStore.ts b/packages/ui/src/features/code-review/reviewViewedStore.ts index 42ffcdbd1f..d8bd13003d 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -2,9 +2,10 @@ import { create } from "zustand"; import { persist } from "zustand/middleware"; // Backstop on persisted size: pruneArchived handles the common case, but tasks -// that are deleted without archiving would otherwise leak forever. Evict the -// least-recently-touched tasks once this cap is exceeded. -const MAX_TASKS = 200; +// that are deleted without archiving would otherwise leak forever. Cap total +// stored entries (≈100 bytes each, so ~400KB) rather than task count, since +// files-per-task varies wildly; evict least-recently-touched tasks past the cap. +const MAX_FILES = 4000; interface ReviewViewedStoreState { // taskId -> file key -> signature of the diff when the file was marked read. @@ -15,6 +16,7 @@ interface ReviewViewedStoreState { interface ReviewViewedStoreActions { // Pass a signature to mark read (at that signature), or null to un-mark. setViewed: (taskId: string, key: string, sig: string | null) => void; + clearTask: (taskId: string) => void; pruneArchived: (archivedTaskIds: Iterable) => void; } @@ -37,12 +39,24 @@ export const useReviewViewedStore = create()( ? { ...rest, [taskId]: taskViewed } : rest; - const taskIds = Object.keys(next); - for (const stale of taskIds.slice(0, taskIds.length - MAX_TASKS)) { - delete next[stale]; + // Evict oldest tasks (front of insertion order) until under the cap, + // never dropping the task just touched. + let total = 0; + for (const id in next) total += Object.keys(next[id]).length; + for (const id of Object.keys(next)) { + if (total <= MAX_FILES) break; + if (id === taskId) continue; + total -= Object.keys(next[id]).length; + delete next[id]; } return { viewed: next }; }), + clearTask: (taskId) => + set((state) => { + if (!(taskId in state.viewed)) return state; + const { [taskId]: _omit, ...rest } = state.viewed; + return { viewed: rest }; + }), pruneArchived: (archivedTaskIds) => set((state) => { let changed = false; From 237323088566bf5dd4b0e92c6700773645b60c05 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Tue, 30 Jun 2026 10:23:37 -0400 Subject: [PATCH 05/21] feat(code-review): show read count in toolbar; tighten read-state cap Display "/ read" next to the file count in the review toolbar, counting only files marked read at their current signature. Lower the persisted-size backstop from 4000 to 500 entries. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b --- .../features/code-review/components/ReviewShell.tsx | 11 +++++++++++ .../features/code-review/components/ReviewToolbar.tsx | 7 +++++++ .../ui/src/features/code-review/reviewViewedStore.ts | 11 ++++++----- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index 2d3645c612..844f93a388 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -141,6 +141,16 @@ export function ReviewShell({ return map; }, [items]); + // Count files marked read at their current signature (changed files don't + // count as read). + const readCount = useMemo(() => { + let count = 0; + for (const [key, sig] of currentSignatures) { + if (viewedRecord[key] === sig) count++; + } + return count; + }, [currentSignatures, viewedRecord]); + // Drop persisted read state for archived tasks so it does not accumulate. // Skip the task being reviewed: archiving it while its review is open must // not wipe the read marks the user is actively working against. @@ -262,6 +272,7 @@ export function ReviewShell({ {fileCount} file{fileCount !== 1 ? "s" : ""} changed + {fileCount > 0 && ( + + {readCount}/{fileCount} read + + )} {effectiveSource && ( file key -> signature of the diff when the file was marked read. From 67be0e141734cda433a010e0a195cdcb7c2417c8 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Tue, 30 Jun 2026 10:23:39 -0400 Subject: [PATCH 06/21] chore(code-review): lower read-state cap to 150 entries Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b --- packages/ui/src/features/code-review/reviewViewedStore.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ui/src/features/code-review/reviewViewedStore.ts b/packages/ui/src/features/code-review/reviewViewedStore.ts index adecdecd74..bd2c9fb1e0 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -3,10 +3,10 @@ import { persist } from "zustand/middleware"; // Backstop on persisted size: clearTask/pruneArchived handle the common cases, // but tasks that are deleted without archiving would otherwise leak forever. -// Cap total stored entries (≈100 bytes each, so ~50KB) rather than task count, +// Cap total stored entries (≈100 bytes each, so ~15KB) rather than task count, // since files-per-task varies wildly; evict least-recently-touched tasks past // the cap. -const MAX_FILES = 500; +const MAX_FILES = 150; interface ReviewViewedStoreState { // taskId -> file key -> signature of the diff when the file was marked read. From b403b614a53f611d5d899636c7b5068b4bf742ff Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Tue, 30 Jun 2026 10:23:41 -0400 Subject: [PATCH 07/21] fix(code-review): make merge detection work for cloud tasks; stabilize local read signature - ReviewShell passed a bare Task to useTaskPrStatus, so cloudPrUrl/ taskRunEnvironment were undefined and the PR-merge read-state clear never fired for cloud tasks. Resolve cloudPrUrl via useCloudPrUrl and pass the run environment, matching the other useTaskPrStatus call sites. - Local read signatures hashed parsed hunks, which change when the hide-whitespace toggle re-fetches a different diff, falsely flipping read files to "Changed". Base the signature on the git blob object ids from the patch index line instead: content-identifying and unaffected by the toggle (falls back to hunk geometry when absent). Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b --- .../features/code-review/components/ReviewShell.tsx | 10 +++++++++- .../code-review/components/reviewItemBuilders.tsx | 9 ++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index 844f93a388..3cf111202c 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -2,6 +2,7 @@ import { WorkerPoolContextProvider } from "@pierre/diffs/react"; import { useService } from "@posthog/di/react"; import type { Task } from "@posthog/shared/domain-types"; import { useArchivedTaskIds } from "@posthog/ui/features/archive/useArchivedTaskIds"; +import { useCloudPrUrl } from "@posthog/ui/features/git-interaction/useCloudPrUrl"; import { useTaskPrStatus } from "@posthog/ui/features/sidebar/useTaskPrStatus"; import { Flex, Spinner, Text } from "@radix-ui/themes"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; @@ -162,7 +163,14 @@ export function ReviewShell({ }, [archivedTaskIds, pruneArchived, taskId]); // Once the PR is merged the diff is settled, so read state is moot — drop it. - const { prState } = useTaskPrStatus(task); + // Cloud tasks resolve their PR via cloudPrUrl, so pass it (and the run + // environment) through or merge detection never fires for them. + const cloudPrUrl = useCloudPrUrl(taskId); + const { prState } = useTaskPrStatus({ + id: taskId, + cloudPrUrl, + taskRunEnvironment: task.latest_run?.environment, + }); const clearReadState = useReviewViewedStore((s) => s.clearTask); useEffect(() => { if (prState === "merged") clearReadState(taskId); diff --git a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx index b8100243b1..fb1db05943 100644 --- a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx +++ b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx @@ -35,7 +35,14 @@ function patchFileSignature( ): string { const cached = signatureCache.get(fileDiff); if (cached !== undefined) return cached; - const sig = contentHash(JSON.stringify(fileDiff.hunks ?? [])); + // Prefer the git blob object ids from the patch `index` line: they identify + // file content directly and are unaffected by the hide-whitespace toggle + // (which re-fetches a different diff that would otherwise change a + // hunk-derived signature). Fall back to hunk geometry when absent. + const sig = + fileDiff.newObjectId || fileDiff.prevObjectId + ? `${fileDiff.prevObjectId ?? ""}:${fileDiff.newObjectId ?? ""}` + : contentHash(JSON.stringify(fileDiff.hunks ?? [])); signatureCache.set(fileDiff, sig); return sig; } From 56ff3f23c056c3416894276c527ed8b072659031 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Tue, 30 Jun 2026 10:23:42 -0400 Subject: [PATCH 08/21] refactor(code-review): consolidate read-state clearing into one clearTasks action clearTask(id) and pruneArchived([id]) did the same single-key delete, and "pruneArchived" read wrong for the merge path. Collapse both into a single clearTasks(ids) action used by archive, merge, and the archived-task backstop. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b --- .../ui/src/features/archive/useArchiveTask.ts | 2 +- .../code-review/components/ReviewShell.tsx | 12 ++++++------ .../features/code-review/reviewViewedStore.ts | 18 ++++++------------ 3 files changed, 13 insertions(+), 19 deletions(-) diff --git a/packages/ui/src/features/archive/useArchiveTask.ts b/packages/ui/src/features/archive/useArchiveTask.ts index 5f42e97454..930097350f 100644 --- a/packages/ui/src/features/archive/useArchiveTask.ts +++ b/packages/ui/src/features/archive/useArchiveTask.ts @@ -124,7 +124,7 @@ function makeOrchestrationDeps( archive: (taskId) => hostClient.archive.archive.mutate({ taskId }).then(() => undefined), clearReadState: (taskId) => - useReviewViewedStore.getState().clearTask(taskId), + useReviewViewedStore.getState().clearTasks([taskId]), logError: (message, error) => log.error(message, error), cache: makeCacheWriter(queryClient, keys), }; diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index 3cf111202c..3d9232641c 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -152,15 +152,16 @@ export function ReviewShell({ return count; }, [currentSignatures, viewedRecord]); + const clearTasks = useReviewViewedStore((s) => s.clearTasks); + // Drop persisted read state for archived tasks so it does not accumulate. // Skip the task being reviewed: archiving it while its review is open must // not wipe the read marks the user is actively working against. const archivedTaskIds = useArchivedTaskIds(); - const pruneArchived = useReviewViewedStore((s) => s.pruneArchived); useEffect(() => { const prunable = [...archivedTaskIds].filter((id) => id !== taskId); - if (prunable.length > 0) pruneArchived(prunable); - }, [archivedTaskIds, pruneArchived, taskId]); + if (prunable.length > 0) clearTasks(prunable); + }, [archivedTaskIds, clearTasks, taskId]); // Once the PR is merged the diff is settled, so read state is moot — drop it. // Cloud tasks resolve their PR via cloudPrUrl, so pass it (and the run @@ -171,10 +172,9 @@ export function ReviewShell({ cloudPrUrl, taskRunEnvironment: task.latest_run?.environment, }); - const clearReadState = useReviewViewedStore((s) => s.clearTask); useEffect(() => { - if (prState === "merged") clearReadState(taskId); - }, [prState, taskId, clearReadState]); + if (prState === "merged") clearTasks([taskId]); + }, [prState, taskId, clearTasks]); const viewedContextValue = useMemo( () => ({ diff --git a/packages/ui/src/features/code-review/reviewViewedStore.ts b/packages/ui/src/features/code-review/reviewViewedStore.ts index bd2c9fb1e0..5eb10eddd8 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -1,8 +1,8 @@ import { create } from "zustand"; import { persist } from "zustand/middleware"; -// Backstop on persisted size: clearTask/pruneArchived handle the common cases, -// but tasks that are deleted without archiving would otherwise leak forever. +// Backstop on persisted size: clearTasks handles the common cases (archive, +// merge), but tasks deleted without archiving would otherwise leak forever. // Cap total stored entries (≈100 bytes each, so ~15KB) rather than task count, // since files-per-task varies wildly; evict least-recently-touched tasks past // the cap. @@ -17,8 +17,8 @@ interface ReviewViewedStoreState { interface ReviewViewedStoreActions { // Pass a signature to mark read (at that signature), or null to un-mark. setViewed: (taskId: string, key: string, sig: string | null) => void; - clearTask: (taskId: string) => void; - pruneArchived: (archivedTaskIds: Iterable) => void; + // Drop read state for the given tasks (archived, merged, or otherwise done). + clearTasks: (taskIds: Iterable) => void; } type ReviewViewedStore = ReviewViewedStoreState & ReviewViewedStoreActions; @@ -52,17 +52,11 @@ export const useReviewViewedStore = create()( } return { viewed: next }; }), - clearTask: (taskId) => - set((state) => { - if (!(taskId in state.viewed)) return state; - const { [taskId]: _omit, ...rest } = state.viewed; - return { viewed: rest }; - }), - pruneArchived: (archivedTaskIds) => + clearTasks: (taskIds) => set((state) => { let changed = false; const next = { ...state.viewed }; - for (const id of archivedTaskIds) { + for (const id of taskIds) { if (id in next) { delete next[id]; changed = true; From 36d3fc4e49ff84f308385203caad228cc94512e6 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Tue, 30 Jun 2026 10:23:44 -0400 Subject: [PATCH 09/21] test+refactor(code-review): read-state tests, stable signatures map, isFileRead, cap 250 - Add unit tests for reviewViewedStore (mark/unmark, clearTasks, entry-cap eviction + active-task retention) and the signature helpers (patch hash, blob-id preference / whitespace stability, fallbacks). - Keep currentSignatures' reference stable across collapse toggles so toggling one file no longer re-renders every ViewedCheckbox via context. - Extract isFileRead() so the toolbar count and the checkbox share one predicate. - Raise the persisted-entry backstop from 150 to 250. Generated-By: PostHog Code Task-Id: c2ac4ecc-f009-4e38-91fb-81f17ccfd91b --- .../code-review/components/ReviewShell.tsx | 20 ++++- .../components/reviewItemBuilders.test.ts | 76 +++++++++++++++++++ .../components/reviewItemBuilders.tsx | 2 +- .../features/code-review/reviewShellParts.tsx | 13 +++- .../code-review/reviewViewedStore.test.ts | 55 ++++++++++++++ .../features/code-review/reviewViewedStore.ts | 4 +- 6 files changed, 164 insertions(+), 6 deletions(-) create mode 100644 packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts create mode 100644 packages/ui/src/features/code-review/reviewViewedStore.test.ts diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index 3d9232641c..94269723bb 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -15,6 +15,7 @@ import { useReviewDraftsStore } from "../reviewDraftsStore"; import { REVIEW_HOST, type ReviewHost } from "../reviewHost"; import { useReviewNavigationStore } from "../reviewNavigationStore"; import type { ReviewListItem, ReviewShellProps } from "../reviewShellParts"; +import { isFileRead } from "../reviewShellParts"; import { ReviewViewedContext } from "../reviewViewedContext"; import { useReviewViewedStore } from "../reviewViewedStore"; import { PendingReviewBar } from "./PendingReviewBar"; @@ -134,11 +135,28 @@ export function ReviewShell({ ); const isExpanded = reviewMode === "expanded"; + // Rebuild the key->signature map from items, but keep the previous reference + // when its contents are unchanged. items get a new identity on every collapse + // toggle; returning a stable map keeps the review context value stable so + // toggling one file doesn't re-render every ViewedCheckbox via context. + const prevSignaturesRef = useRef>(new Map()); const currentSignatures = useMemo(() => { const map = new Map(); for (const item of items) { if (item.sig !== undefined) map.set(item.key, item.sig); } + const prev = prevSignaturesRef.current; + let unchanged = prev.size === map.size; + if (unchanged) { + for (const [key, sig] of map) { + if (prev.get(key) !== sig) { + unchanged = false; + break; + } + } + } + if (unchanged) return prev; + prevSignaturesRef.current = map; return map; }, [items]); @@ -147,7 +165,7 @@ export function ReviewShell({ const readCount = useMemo(() => { let count = 0; for (const [key, sig] of currentSignatures) { - if (viewedRecord[key] === sig) count++; + if (isFileRead(viewedRecord[key], sig)) count++; } return count; }, [currentSignatures, viewedRecord]); diff --git a/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts b/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts new file mode 100644 index 0000000000..7b27b5c64f --- /dev/null +++ b/packages/ui/src/features/code-review/components/reviewItemBuilders.test.ts @@ -0,0 +1,76 @@ +import type { ChangedFile } from "@posthog/shared/domain-types"; +import { describe, expect, it } from "vitest"; +import { changedFileSignature, patchFileSignature } from "./reviewItemBuilders"; + +const changedFile = (over: Partial): ChangedFile => ({ + path: "a.ts", + status: "modified", + ...over, +}); + +describe("changedFileSignature", () => { + it("differs when the patch content differs", () => { + const a = changedFileSignature( + changedFile({ patch: "@@ -1 +1 @@\n-x\n+y" }), + ); + const b = changedFileSignature( + changedFile({ patch: "@@ -1 +1 @@\n-x\n+z" }), + ); + expect(a).not.toBe(b); + }); + + it("falls back to status + line counts when there is no patch", () => { + const a = changedFileSignature( + changedFile({ linesAdded: 1, linesRemoved: 2 }), + ); + const b = changedFileSignature( + changedFile({ linesAdded: 1, linesRemoved: 2 }), + ); + const c = changedFileSignature( + changedFile({ linesAdded: 3, linesRemoved: 2 }), + ); + expect(a).toBe(b); + expect(a).not.toBe(c); + }); +}); + +describe("patchFileSignature", () => { + // biome-ignore lint/suspicious/noExplicitAny: minimal pierre FileDiff stub + const fileDiff = (over: Record): any => ({ + hunks: [], + ...over, + }); + + it("uses git blob object ids and ignores hunk content (whitespace-stable)", () => { + // Same blob ids, different parsed hunks (as the hide-whitespace toggle + // would produce) must yield the same signature. + const a = patchFileSignature( + fileDiff({ prevObjectId: "aaa", newObjectId: "bbb", hunks: [{ x: 1 }] }), + ); + const b = patchFileSignature( + fileDiff({ + prevObjectId: "aaa", + newObjectId: "bbb", + hunks: [{ x: 2, y: 3 }], + }), + ); + expect(a).toBe("aaa:bbb"); + expect(b).toBe("aaa:bbb"); + }); + + it("changes when the new blob id changes", () => { + const a = patchFileSignature( + fileDiff({ prevObjectId: "aaa", newObjectId: "bbb" }), + ); + const b = patchFileSignature( + fileDiff({ prevObjectId: "aaa", newObjectId: "ccc" }), + ); + expect(a).not.toBe(b); + }); + + it("falls back to hashing hunks when object ids are absent", () => { + const a = patchFileSignature(fileDiff({ hunks: [{ additionLines: 1 }] })); + const b = patchFileSignature(fileDiff({ hunks: [{ additionLines: 2 }] })); + expect(a).not.toBe(b); + }); +}); diff --git a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx index fb1db05943..8737720064 100644 --- a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx +++ b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx @@ -30,7 +30,7 @@ export function changedFileSignature(file: ChangedFile): string { return sig; } -function patchFileSignature( +export function patchFileSignature( fileDiff: ReturnType[number]["files"][number], ): string { const cached = signatureCache.get(fileDiff); diff --git a/packages/ui/src/features/code-review/reviewShellParts.tsx b/packages/ui/src/features/code-review/reviewShellParts.tsx index ec4c1510fd..4e76b74360 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -248,6 +248,15 @@ export function FileHeaderRow({ ); } +// A file is read when its stored signature matches the current diff signature; +// a stored signature that no longer matches means the diff changed since. +export function isFileRead( + storedSig: string | undefined, + currentSig: string, +): boolean { + return storedSig === currentSig; +} + function ViewedCheckbox({ viewedKey }: { viewedKey: string }) { const ctx = useReviewViewedContext(); if (!ctx) return null; @@ -256,8 +265,8 @@ function ViewedCheckbox({ viewedKey }: { viewedKey: string }) { if (current === undefined) return null; const stored = ctx.viewedRecord[viewedKey]; - const read = stored === current; - const changed = stored !== undefined && stored !== current; + const read = isFileRead(stored, current); + const changed = stored !== undefined && !read; return ( ); diff --git a/packages/ui/src/features/code-review/reviewViewedContext.ts b/packages/ui/src/features/code-review/reviewViewedContext.ts index 1e19dc10e0..0bb12ec827 100644 --- a/packages/ui/src/features/code-review/reviewViewedContext.ts +++ b/packages/ui/src/features/code-review/reviewViewedContext.ts @@ -1,11 +1,11 @@ import { createContext, useContext } from "react"; export interface ReviewViewedContextValue { - // key -> signature of the diff when the file was marked read + // key -> signature of the diff when the file was marked viewed viewedRecord: Record; // key -> current signature of the diff being shown currentSignatures: Map; - // Pass a signature to mark read (at that signature), or null to un-mark. + // Pass a signature to mark viewed (at that signature), or null to un-mark. toggleViewed: (key: string, sig: string | null) => void; } diff --git a/packages/ui/src/features/code-review/reviewViewedStore.ts b/packages/ui/src/features/code-review/reviewViewedStore.ts index 7ca8d89b5b..fc000c8ffc 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -9,15 +9,15 @@ import { persist } from "zustand/middleware"; const MAX_FILES = 250; interface ReviewViewedStoreState { - // taskId -> file key -> signature of the diff when the file was marked read. + // taskId -> file key -> signature of the diff when the file was marked viewed. // Insertion order is treated as recency (touched tasks re-inserted last). viewed: Record>; } interface ReviewViewedStoreActions { - // Pass a signature to mark read (at that signature), or null to un-mark. + // Pass a signature to mark viewed (at that signature), or null to un-mark. setViewed: (taskId: string, key: string, sig: string | null) => void; - // Drop read state for the given tasks (archived, merged, or otherwise done). + // Drop viewed state for the given tasks (archived, merged, or otherwise done). clearTasks: (taskIds: Iterable) => void; } @@ -69,7 +69,7 @@ export const useReviewViewedStore = create()( name: "review-viewed-storage", version: 1, // v0 stored booleans without a signature; drop them so files re-resolve - // their read state under the signature-aware model. + // their viewed state under the signature-aware model. migrate: (persisted, version) => { if (version < 1) return { viewed: {} }; return persisted as ReviewViewedStoreState; From fd7397998bee8510eda9496c529af2d27de02987 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 2 Jul 2026 13:53:16 -0400 Subject: [PATCH 12/21] chore(code-review): strip useless code comments Generated-By: PostHog Code Task-Id: 4ccb2163-f475-4055-b186-3216daed734f --- .../code-review/components/ReviewPage.tsx | 2 -- .../code-review/components/ReviewShell.tsx | 16 +++------------- .../components/reviewItemBuilders.tsx | 4 ---- .../features/code-review/reviewShellParts.tsx | 7 ------- .../features/code-review/reviewViewedContext.ts | 3 --- .../features/code-review/reviewViewedStore.ts | 17 +++-------------- 6 files changed, 6 insertions(+), 43 deletions(-) diff --git a/packages/ui/src/features/code-review/components/ReviewPage.tsx b/packages/ui/src/features/code-review/components/ReviewPage.tsx index c239314a04..9851927ad7 100644 --- a/packages/ui/src/features/code-review/components/ReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/ReviewPage.tsx @@ -307,8 +307,6 @@ function LocalReviewContent({ [filesByKey, stageToggle], ); - // Diff signatures back the viewed state. Computed once per file load/refetch - // (the parsed-file arrays only change identity then), not per item rebuild. const currentSignatures = useMemo(() => { const map = new Map(); for (const f of stagedParsedFiles) { diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index 046909519d..dbdc3c5d48 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -137,8 +137,6 @@ export function ReviewShell({ ); const isExpanded = reviewMode === "expanded"; - // Count files marked viewed at their current signature (changed files don't - // count as viewed). const viewedCount = useMemo(() => { let count = 0; for (const [key, sig] of currentSignatures) { @@ -147,11 +145,9 @@ export function ReviewShell({ return count; }, [currentSignatures, viewedRecord]); - // When the panel first opens for a task, collapse files that are already - // viewed (mirrors GitHub). Runs once per task, once signatures have loaded, - // so it doesn't fight the user manually re-expanding a viewed file - // afterwards. Files that changed since being viewed stay expanded so the new - // diff is visible. + // Collapse already-viewed files on first open per task (mirrors GitHub). + // Skips on re-opens: seededTaskRef prevents re-collapsing files the user + // has manually expanded. Files changed since viewed stay expanded. const seededTaskRef = useRef(null); useEffect(() => { if (seededTaskRef.current === taskId) return; @@ -166,18 +162,12 @@ export function ReviewShell({ const clearTasks = useReviewViewedStore((s) => s.clearTasks); - // Drop persisted viewed state for archived tasks so it does not accumulate. - // Skip the task being reviewed: archiving it while its review is open must - // not wipe the viewed marks the user is actively working against. const archivedTaskIds = useArchivedTaskIds(); useEffect(() => { const prunable = [...archivedTaskIds].filter((id) => id !== taskId); if (prunable.length > 0) clearTasks(prunable); }, [archivedTaskIds, clearTasks, taskId]); - // Once the PR is merged the diff is settled, so viewed state is moot — drop it. - // Cloud tasks resolve their PR via cloudPrUrl, so pass it (and the run - // environment) through or merge detection never fires for them. const cloudPrUrl = useCloudPrUrl(taskId); const { prState } = useTaskPrStatus({ id: taskId, diff --git a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx index c5333d00d5..07285d2999 100644 --- a/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx +++ b/packages/ui/src/features/code-review/components/reviewItemBuilders.tsx @@ -11,10 +11,6 @@ import type { ReviewListItem } from "../reviewShellParts"; import type { DiffOptions } from "../types"; import { PatchRow, RemoteRow, UntrackedRow } from "./ReviewRows"; -// Signatures fingerprint a file's diff so viewed state can detect when the -// diff changed after the user marked it viewed. Callers compute them once per -// file load/refetch (memoized on the files array), not per render. - // Prefer the unified patch (changes whenever upstream content does); fall back // to status + line counts when no patch is available. export function changedFileSignature(file: ChangedFile): string { diff --git a/packages/ui/src/features/code-review/reviewShellParts.tsx b/packages/ui/src/features/code-review/reviewShellParts.tsx index e46132f6f3..c8a4da8224 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -88,8 +88,6 @@ function useViewedState( useReviewViewedStore((s) => s.viewed[taskId]) ?? EMPTY_VIEWED_RECORD; const setViewed = useReviewViewedStore((s) => s.setViewed); - // `nextSig` is the signature to store, or null to clear the viewed mark. - // Marking a file viewed collapses it; un-marking expands it (mirrors GitHub). const toggleViewed = useCallback( (key: string, nextSig: string | null) => { setViewed(taskId, key, nextSig); @@ -178,8 +176,6 @@ export interface ReviewShellProps { isEmpty: boolean; items: ReviewListItem[]; itemIndexByFilePath: Map; - // key -> current diff signature, memoized by the page on its files data so - // it only changes when the underlying diff actually changes. currentSignatures: Map; viewedRecord: Record; onToggleViewed: (key: string, sig: string | null) => void; @@ -265,9 +261,6 @@ export function FileHeaderRow({ ); } -// A file is viewed when its stored signature matches the current diff -// signature; a stored signature that no longer matches means the diff changed -// since the user viewed it. export function isFileViewed( storedSig: string | undefined, currentSig: string, diff --git a/packages/ui/src/features/code-review/reviewViewedContext.ts b/packages/ui/src/features/code-review/reviewViewedContext.ts index 0bb12ec827..888017fcdc 100644 --- a/packages/ui/src/features/code-review/reviewViewedContext.ts +++ b/packages/ui/src/features/code-review/reviewViewedContext.ts @@ -1,11 +1,8 @@ import { createContext, useContext } from "react"; export interface ReviewViewedContextValue { - // key -> signature of the diff when the file was marked viewed viewedRecord: Record; - // key -> current signature of the diff being shown currentSignatures: Map; - // Pass a signature to mark viewed (at that signature), or null to un-mark. toggleViewed: (key: string, sig: string | null) => void; } diff --git a/packages/ui/src/features/code-review/reviewViewedStore.ts b/packages/ui/src/features/code-review/reviewViewedStore.ts index fc000c8ffc..dc275e7dd5 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -1,23 +1,17 @@ import { create } from "zustand"; import { persist } from "zustand/middleware"; -// Backstop on persisted size: clearTasks handles the common cases (archive, -// merge), but tasks deleted without archiving would otherwise leak forever. -// Cap total stored entries (≈100 bytes each, so ~25KB) rather than task count, -// since files-per-task varies wildly; evict least-recently-touched tasks past -// the cap. +// Backstop: tasks deleted without archiving would otherwise accumulate forever. +// Cap by file count (not task count) since files-per-task varies; evict +// least-recently-touched tasks past the cap. const MAX_FILES = 250; interface ReviewViewedStoreState { - // taskId -> file key -> signature of the diff when the file was marked viewed. - // Insertion order is treated as recency (touched tasks re-inserted last). viewed: Record>; } interface ReviewViewedStoreActions { - // Pass a signature to mark viewed (at that signature), or null to un-mark. setViewed: (taskId: string, key: string, sig: string | null) => void; - // Drop viewed state for the given tasks (archived, merged, or otherwise done). clearTasks: (taskIds: Iterable) => void; } @@ -33,15 +27,12 @@ export const useReviewViewedStore = create()( if (sig === null) delete taskViewed[key]; else taskViewed[key] = sig; - // Re-insert the touched task last so it is evicted last. const { [taskId]: _omit, ...rest } = state.viewed; const next = Object.keys(taskViewed).length > 0 ? { ...rest, [taskId]: taskViewed } : rest; - // Evict oldest tasks (front of insertion order) until under the cap, - // never dropping the task just touched. let total = 0; for (const id in next) total += Object.keys(next[id]).length; for (const id of Object.keys(next)) { @@ -68,8 +59,6 @@ export const useReviewViewedStore = create()( { name: "review-viewed-storage", version: 1, - // v0 stored booleans without a signature; drop them so files re-resolve - // their viewed state under the signature-aware model. migrate: (persisted, version) => { if (version < 1) return { viewed: {} }; return persisted as ReviewViewedStoreState; From 33c1984bd6d2657614958357b3508c0b19b8bee2 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 2 Jul 2026 14:01:25 -0400 Subject: [PATCH 13/21] refactor(code-review): simplify viewed store and resolve rebase conflicts - Remove LRU eviction from setViewed: clearTasks on archive/merge is the real cleanup path; simple object mutation is enough - Remove the two LRU-specific tests - Resolve rebase conflicts (clearTerminalStates from main + clearViewedState from this branch both land in archiveOrchestration) Generated-By: PostHog Code Task-Id: 4ccb2163-f475-4055-b186-3216daed734f --- .../code-review/reviewViewedStore.test.ts | 12 ---------- .../features/code-review/reviewViewedStore.ts | 23 +++---------------- 2 files changed, 3 insertions(+), 32 deletions(-) diff --git a/packages/ui/src/features/code-review/reviewViewedStore.test.ts b/packages/ui/src/features/code-review/reviewViewedStore.test.ts index 37e166bddd..9223197c9b 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.test.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.test.ts @@ -40,16 +40,4 @@ describe("reviewViewedStore", () => { expect(viewed()).toBe(before); }); - it("evicts least-recently-touched tasks past the entry cap", () => { - for (let i = 0; i < 260; i++) setViewed(`t${i}`, "f", "s"); - expect(Object.keys(viewed()).length).toBe(250); - expect(viewed().t0).toBeUndefined(); // oldest evicted - expect(viewed().t259).toBeDefined(); // most recent kept - }); - - it("never evicts the task currently being marked, even past the cap", () => { - for (let i = 0; i < 250; i++) setViewed(`old${i}`, "f", "s"); - for (let f = 0; f < 300; f++) setViewed("big", `f${f}`, "s"); - expect(Object.keys(viewed().big ?? {}).length).toBe(300); - }); }); diff --git a/packages/ui/src/features/code-review/reviewViewedStore.ts b/packages/ui/src/features/code-review/reviewViewedStore.ts index dc275e7dd5..eb63bdc45d 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -1,11 +1,6 @@ import { create } from "zustand"; import { persist } from "zustand/middleware"; -// Backstop: tasks deleted without archiving would otherwise accumulate forever. -// Cap by file count (not task count) since files-per-task varies; evict -// least-recently-touched tasks past the cap. -const MAX_FILES = 250; - interface ReviewViewedStoreState { viewed: Record>; } @@ -26,21 +21,9 @@ export const useReviewViewedStore = create()( const taskViewed = { ...(state.viewed[taskId] ?? {}) }; if (sig === null) delete taskViewed[key]; else taskViewed[key] = sig; - - const { [taskId]: _omit, ...rest } = state.viewed; - const next = - Object.keys(taskViewed).length > 0 - ? { ...rest, [taskId]: taskViewed } - : rest; - - let total = 0; - for (const id in next) total += Object.keys(next[id]).length; - for (const id of Object.keys(next)) { - if (total <= MAX_FILES) break; - if (id === taskId) continue; - total -= Object.keys(next[id]).length; - delete next[id]; - } + const next = { ...state.viewed }; + if (Object.keys(taskViewed).length > 0) next[taskId] = taskViewed; + else delete next[taskId]; return { viewed: next }; }), clearTasks: (taskIds) => From 2b27043bdb2da9353870ea38c725edea86936195 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 2 Jul 2026 14:07:05 -0400 Subject: [PATCH 14/21] fix(code-review): fix trailing blank line in store test Generated-By: PostHog Code Task-Id: 4ccb2163-f475-4055-b186-3216daed734f --- packages/ui/src/features/code-review/reviewViewedStore.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/ui/src/features/code-review/reviewViewedStore.test.ts b/packages/ui/src/features/code-review/reviewViewedStore.test.ts index 9223197c9b..554eea5df0 100644 --- a/packages/ui/src/features/code-review/reviewViewedStore.test.ts +++ b/packages/ui/src/features/code-review/reviewViewedStore.test.ts @@ -39,5 +39,4 @@ describe("reviewViewedStore", () => { clearTasks(["unknown"]); expect(viewed()).toBe(before); }); - }); From a6141587d4919878a2ddfa2595016639317dbed6 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Fri, 17 Jul 2026 10:02:16 -0400 Subject: [PATCH 15/21] fix(code-review): preserve viewed file collapse on navigation Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8 --- .../code-review/components/ReviewShell.tsx | 8 +++++++- .../task-detail/components/ChangesPanel.tsx | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index ca6c3a9a0a..e93c0e4f44 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -211,7 +211,11 @@ export function ReviewShell({ const targetIndex = itemIndexByFilePath.get(scrollRequest); if (targetIndex === undefined) return; - onUncollapseFile?.(scrollRequest); + const currentSignature = currentSignatures.get(scrollRequest); + const viewed = + currentSignature !== undefined && + isFileViewed(viewedRecord[scrollRequest], currentSignature); + if (!viewed) onUncollapseFile?.(scrollRequest); requestAnimationFrame(() => { listRef.current?.scrollToIndex(targetIndex, { align: "start" }); setActiveFilePath(taskId, scrollRequest); @@ -219,11 +223,13 @@ export function ReviewShell({ }); }, [ clearScrollRequest, + currentSignatures, itemIndexByFilePath, onUncollapseFile, scrollRequest, setActiveFilePath, taskId, + viewedRecord, ]); const lastActiveRef = useRef(null); diff --git a/packages/ui/src/features/task-detail/components/ChangesPanel.tsx b/packages/ui/src/features/task-detail/components/ChangesPanel.tsx index cfc14c578b..d266ab9bff 100644 --- a/packages/ui/src/features/task-detail/components/ChangesPanel.tsx +++ b/packages/ui/src/features/task-detail/components/ChangesPanel.tsx @@ -1,5 +1,6 @@ import { ArrowCounterClockwiseIcon, + CheckSquare, CodeIcon, CopyIcon, FilePlus, @@ -29,6 +30,8 @@ import { TreeFileRow } from "../../../primitives/TreeDirectoryRow"; import { track } from "../../../shell/analytics"; import { useEffectiveDiffSource } from "../../code-review/hooks/useEffectiveDiffSource"; import { useReviewNavigationStore } from "../../code-review/reviewNavigationStore"; +import { isFileViewed } from "../../code-review/reviewShellParts"; +import { useReviewViewedContext } from "../../code-review/reviewViewedContext"; import { useExternalAppAction } from "../../external-apps/useExternalAppAction"; import { useExternalApps } from "../../external-apps/useExternalApps"; import { @@ -107,6 +110,7 @@ function ChangedFileItem({ const { detectedApps } = useExternalApps(); const workspace = useWorkspace(taskId); const { openForFile } = useFileContextMenu(); + const viewedContext = useReviewViewedContext(); const [isHovered, setIsHovered] = useState(false); const [isDropdownOpen, setIsDropdownOpen] = useState(false); @@ -117,6 +121,10 @@ function ChangedFileItem({ const fileName = file.path.split("/").pop() || file.path; const fullPath = repoPath ? `${repoPath}/${file.path}` : file.path; const indicator = getStatusIndicator(file.status); + const currentSignature = viewedContext?.currentSignatures.get(fileKey); + const viewed = + currentSignature !== undefined && + isFileViewed(viewedContext?.viewedRecord[fileKey], currentSignature); const handleClick = () => { track(ANALYTICS_EVENTS.FILE_DIFF_VIEWED, { @@ -276,6 +284,14 @@ function ChangedFileItem({ > {indicator.label} + {viewed && ( + + )} ); From 24e63b59d77972d53d6781f01ca0b1c0bcdb4fb9 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Fri, 17 Jul 2026 10:25:52 -0400 Subject: [PATCH 16/21] fix(code-review): keep jump target selected Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8 --- .../features/code-review/components/ReviewShell.tsx | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index e93c0e4f44..d17628c173 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -127,6 +127,8 @@ export function ReviewShell({ const reviewHost = useService(REVIEW_HOST); const taskId = task.id; const listRef = useRef(null); + const lastActiveRef = useRef(null); + const navigationLockUntilRef = useRef(0); const workerFactory = useCallback( () => reviewHost.diffWorkerFactory(), @@ -215,9 +217,11 @@ export function ReviewShell({ const viewed = currentSignature !== undefined && isFileViewed(viewedRecord[scrollRequest], currentSignature); + navigationLockUntilRef.current = Date.now() + 500; if (!viewed) onUncollapseFile?.(scrollRequest); requestAnimationFrame(() => { listRef.current?.scrollToIndex(targetIndex, { align: "start" }); + lastActiveRef.current = scrollRequest; setActiveFilePath(taskId, scrollRequest); clearScrollRequest(taskId); }); @@ -232,9 +236,10 @@ export function ReviewShell({ viewedRecord, ]); - const lastActiveRef = useRef(null); const handleScroll = useCallback( (offset: number) => { + if (Date.now() < navigationLockUntilRef.current) return; + navigationLockUntilRef.current = 0; const handle = listRef.current; if (!handle) return; const index = handle.findItemIndex(offset); @@ -247,6 +252,10 @@ export function ReviewShell({ [items, setActiveFilePath, taskId], ); + const handleUserScrollIntent = useCallback(() => { + navigationLockUntilRef.current = 0; + }, []); + const renderItem = useCallback( (item: ReviewListItem) => (
{renderItem} From fc6dbe933c4e3e5b5f208572e371d397865b2e80 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Fri, 17 Jul 2026 10:29:03 -0400 Subject: [PATCH 17/21] fix(code-review): retain explicit jump selection Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8 --- .../code-review/components/ReviewShell.tsx | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index d17628c173..7fe9a5f458 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -128,7 +128,7 @@ export function ReviewShell({ const taskId = task.id; const listRef = useRef(null); const lastActiveRef = useRef(null); - const navigationLockUntilRef = useRef(0); + const navigationLockedRef = useRef(false); const workerFactory = useCallback( () => reviewHost.diffWorkerFactory(), @@ -217,7 +217,7 @@ export function ReviewShell({ const viewed = currentSignature !== undefined && isFileViewed(viewedRecord[scrollRequest], currentSignature); - navigationLockUntilRef.current = Date.now() + 500; + navigationLockedRef.current = true; if (!viewed) onUncollapseFile?.(scrollRequest); requestAnimationFrame(() => { listRef.current?.scrollToIndex(targetIndex, { align: "start" }); @@ -238,8 +238,7 @@ export function ReviewShell({ const handleScroll = useCallback( (offset: number) => { - if (Date.now() < navigationLockUntilRef.current) return; - navigationLockUntilRef.current = 0; + if (navigationLockedRef.current) return; const handle = listRef.current; if (!handle) return; const index = handle.findItemIndex(offset); @@ -253,7 +252,7 @@ export function ReviewShell({ ); const handleUserScrollIntent = useCallback(() => { - navigationLockUntilRef.current = 0; + navigationLockedRef.current = false; }, []); const renderItem = useCallback( @@ -314,7 +313,13 @@ export function ReviewShell({ defaultBranch={defaultBranch} /> - + {isLoading ? ( {renderItem} From 6cdf7747afdded137c388709f746365e58c1cb87 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Fri, 17 Jul 2026 10:32:56 -0400 Subject: [PATCH 18/21] fix(code-review): navigate with rendered file anchors Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8 --- .../code-review/components/ReviewShell.tsx | 81 ++++++++++++------- .../code-review/reviewShellParts.test.tsx | 39 ++++++++- .../features/code-review/reviewShellParts.tsx | 26 ++++++ 3 files changed, 116 insertions(+), 30 deletions(-) diff --git a/packages/ui/src/features/code-review/components/ReviewShell.tsx b/packages/ui/src/features/code-review/components/ReviewShell.tsx index 7fe9a5f458..3e03271bfc 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -15,7 +15,11 @@ import { useReviewDraftsStore } from "../reviewDraftsStore"; import { REVIEW_HOST, type ReviewHost } from "../reviewHost"; import { useReviewNavigationStore } from "../reviewNavigationStore"; import type { ReviewListItem, ReviewShellProps } from "../reviewShellParts"; -import { isFileViewed } from "../reviewShellParts"; +import { + findActiveScrollKey, + findRenderedScrollAnchor, + isFileViewed, +} from "../reviewShellParts"; import { ReviewViewedContext } from "../reviewViewedContext"; import { useReviewViewedStore } from "../reviewViewedStore"; import { PendingReviewBar } from "./PendingReviewBar"; @@ -127,8 +131,10 @@ export function ReviewShell({ const reviewHost = useService(REVIEW_HOST); const taskId = task.id; const listRef = useRef(null); + const listContainerRef = useRef(null); const lastActiveRef = useRef(null); - const navigationLockedRef = useRef(false); + const pendingNavigationRef = useRef(null); + const navigationFrameRef = useRef(null); const workerFactory = useCallback( () => reviewHost.diffWorkerFactory(), @@ -203,6 +209,9 @@ export function ReviewShell({ useEffect(() => { return () => { + if (navigationFrameRef.current !== null) { + cancelAnimationFrame(navigationFrameRef.current); + } clearTask(taskId); useReviewDraftsStore.getState().clearDrafts(taskId); }; @@ -217,14 +226,37 @@ export function ReviewShell({ const viewed = currentSignature !== undefined && isFileViewed(viewedRecord[scrollRequest], currentSignature); - navigationLockedRef.current = true; + if (navigationFrameRef.current !== null) { + cancelAnimationFrame(navigationFrameRef.current); + } + pendingNavigationRef.current = scrollRequest; if (!viewed) onUncollapseFile?.(scrollRequest); - requestAnimationFrame(() => { + + const scrollToAnchor = (remainingAttempts: number) => { listRef.current?.scrollToIndex(targetIndex, { align: "start" }); - lastActiveRef.current = scrollRequest; - setActiveFilePath(taskId, scrollRequest); - clearScrollRequest(taskId); - }); + navigationFrameRef.current = requestAnimationFrame(() => { + const root = listContainerRef.current; + const anchor = root + ? findRenderedScrollAnchor(root, scrollRequest) + : null; + + if (!anchor && remainingAttempts > 0) { + scrollToAnchor(remainingAttempts - 1); + return; + } + + anchor?.scrollIntoView({ block: "start", inline: "nearest" }); + lastActiveRef.current = scrollRequest; + setActiveFilePath(taskId, scrollRequest); + clearScrollRequest(taskId); + navigationFrameRef.current = requestAnimationFrame(() => { + pendingNavigationRef.current = null; + navigationFrameRef.current = null; + }); + }); + }; + + scrollToAnchor(5); }, [ clearScrollRequest, currentSignatures, @@ -236,24 +268,17 @@ export function ReviewShell({ viewedRecord, ]); - const handleScroll = useCallback( - (offset: number) => { - if (navigationLockedRef.current) return; - const handle = listRef.current; - if (!handle) return; - const index = handle.findItemIndex(offset); - const item = items[index]; - const scrollKey = item?.scrollKey; - if (!scrollKey || scrollKey === lastActiveRef.current) return; - lastActiveRef.current = scrollKey; - setActiveFilePath(taskId, scrollKey); - }, - [items, setActiveFilePath, taskId], - ); - - const handleUserScrollIntent = useCallback(() => { - navigationLockedRef.current = false; - }, []); + const handleScroll = useCallback(() => { + if (pendingNavigationRef.current !== null) return; + const scrollRoot = listContainerRef.current?.querySelector( + ".pierre-scroll-root", + ); + if (!scrollRoot) return; + const scrollKey = findActiveScrollKey(scrollRoot); + if (!scrollKey || scrollKey === lastActiveRef.current) return; + lastActiveRef.current = scrollKey; + setActiveFilePath(taskId, scrollKey); + }, [setActiveFilePath, taskId]); const renderItem = useCallback( (item: ReviewListItem) => ( @@ -314,11 +339,9 @@ export function ReviewShell({ /> {isLoading ? ( ({ FileIcon: () => , })); -import { DeferredDiffPlaceholder, DiffFileHeader } from "./reviewShellParts"; +import { + DeferredDiffPlaceholder, + DiffFileHeader, + findActiveScrollKey, + findRenderedScrollAnchor, +} from "./reviewShellParts"; type FileDiffMetadata = import("@pierre/diffs/react").FileDiffMetadata; @@ -107,3 +112,35 @@ describe.each([ expect(text.indexOf("2 comments")).toBeLessThan(text.indexOf(additions)); }); }); + +function setRect(element: HTMLElement, top: number, bottom: number) { + element.getBoundingClientRect = vi.fn(() => ({ top, bottom }) as DOMRect); +} + +describe("review scroll anchors", () => { + it("finds a rendered anchor by its exact file key", () => { + const root = document.createElement("div"); + const anchor = document.createElement("div"); + anchor.dataset.scrollKey = "src/[id]/file.ts"; + root.append(anchor); + + expect(findRenderedScrollAnchor(root, "src/[id]/file.ts")).toBe(anchor); + }); + + it("selects the first rendered file crossing the scroll root top", () => { + const root = document.createElement("div"); + const above = document.createElement("div"); + const active = document.createElement("div"); + const below = document.createElement("div"); + above.dataset.scrollKey = "above.ts"; + active.dataset.scrollKey = "active.ts"; + below.dataset.scrollKey = "below.ts"; + root.append(above, active, below); + setRect(root, 100, 500); + setRect(above, 20, 90); + setRect(active, 80, 180); + setRect(below, 180, 280); + + expect(findActiveScrollKey(root)).toBe("active.ts"); + }); +}); diff --git a/packages/ui/src/features/code-review/reviewShellParts.tsx b/packages/ui/src/features/code-review/reviewShellParts.tsx index 1fae0984b1..0a273fc336 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -34,6 +34,32 @@ export { } from "@posthog/core/code-review/reviewShellGeometry"; const STICKY_HEADER_CSS = `[data-diffs-header] { position: sticky; top: 0; z-index: 1; background: var(--gray-2); }`; +const SCROLL_ANCHOR_SELECTOR = "[data-scroll-key]"; + +export function findRenderedScrollAnchor( + root: HTMLElement, + scrollKey: string, +): HTMLElement | null { + for (const anchor of root.querySelectorAll( + SCROLL_ANCHOR_SELECTOR, + )) { + if (anchor.dataset.scrollKey === scrollKey) return anchor; + } + return null; +} + +export function findActiveScrollKey(root: HTMLElement): string | null { + const rootTop = root.getBoundingClientRect().top; + for (const anchor of root.querySelectorAll( + SCROLL_ANCHOR_SELECTOR, + )) { + const scrollKey = anchor.dataset.scrollKey; + if (scrollKey && anchor.getBoundingClientRect().bottom > rootTop + 1) { + return scrollKey; + } + } + return null; +} export function useDiffOptions() { const viewMode = useDiffViewerStore((s) => s.viewMode); From 4983275b1ecbc97e137f15fd381a84b0584a86de Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Fri, 17 Jul 2026 10:36:30 -0400 Subject: [PATCH 19/21] fix(code-review): select jump target by anchor start Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8 --- .../code-review/reviewShellParts.test.tsx | 19 ++++++++++++++++++- .../features/code-review/reviewShellParts.tsx | 10 +++++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/packages/ui/src/features/code-review/reviewShellParts.test.tsx b/packages/ui/src/features/code-review/reviewShellParts.test.tsx index 95e4e5bd04..c51b63a2f9 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.test.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.test.tsx @@ -127,7 +127,7 @@ describe("review scroll anchors", () => { expect(findRenderedScrollAnchor(root, "src/[id]/file.ts")).toBe(anchor); }); - it("selects the first rendered file crossing the scroll root top", () => { + it("selects the last file starting at or above the scroll root top", () => { const root = document.createElement("div"); const above = document.createElement("div"); const active = document.createElement("div"); @@ -143,4 +143,21 @@ describe("review scroll anchors", () => { expect(findActiveScrollKey(root)).toBe("active.ts"); }); + + it("does not select a tall expanded file above the jump target", () => { + const root = document.createElement("div"); + const expandedAbove = document.createElement("div"); + const target = document.createElement("div"); + const below = document.createElement("div"); + expandedAbove.dataset.scrollKey = "expanded-above.ts"; + target.dataset.scrollKey = "target.ts"; + below.dataset.scrollKey = "below.ts"; + root.append(expandedAbove, target, below); + setRect(root, 100, 500); + setRect(expandedAbove, -1000, 500); + setRect(target, 100, 180); + setRect(below, 180, 260); + + expect(findActiveScrollKey(root)).toBe("target.ts"); + }); }); diff --git a/packages/ui/src/features/code-review/reviewShellParts.tsx b/packages/ui/src/features/code-review/reviewShellParts.tsx index 0a273fc336..4e2a1d73e2 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -50,15 +50,19 @@ export function findRenderedScrollAnchor( export function findActiveScrollKey(root: HTMLElement): string | null { const rootTop = root.getBoundingClientRect().top; + let activeScrollKey: string | null = null; for (const anchor of root.querySelectorAll( SCROLL_ANCHOR_SELECTOR, )) { const scrollKey = anchor.dataset.scrollKey; - if (scrollKey && anchor.getBoundingClientRect().bottom > rootTop + 1) { - return scrollKey; + if (!scrollKey) continue; + if (anchor.getBoundingClientRect().top <= rootTop + 1) { + activeScrollKey = scrollKey; + continue; } + return activeScrollKey ?? scrollKey; } - return null; + return activeScrollKey; } export function useDiffOptions() { From 3c154e5b92296fc72a7955e6b03e2a5badf0a555 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Fri, 17 Jul 2026 10:40:03 -0400 Subject: [PATCH 20/21] fix(code-review): clean up viewed checkbox rendering Generated-By: PostHog Code Task-Id: 71e782ac-16eb-409f-a5d3-2aa4ad9117f8 --- .../features/code-review/reviewShellParts.tsx | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/ui/src/features/code-review/reviewShellParts.tsx b/packages/ui/src/features/code-review/reviewShellParts.tsx index 4e2a1d73e2..fa1327522f 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -252,8 +252,6 @@ export function FileHeaderRow({ viewedKey?: string; }) { return ( - // The toggle target is a button; the open-file / read controls sit - // alongside it (not nested inside it, which would be invalid HTML).