From 635f254c73d81d5d4192f5035e365a42a4c5937a Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 18 Jun 2026 14:40:53 -0400 Subject: [PATCH 01/10] 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 | 100 +++++++++++++++- .../code-review/reviewViewedContext.ts | 13 +++ .../features/code-review/reviewViewedStore.ts | 39 +++++++ 8 files changed, 247 insertions(+), 52 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 cb93df86c6..4d60953ce1 100644 --- a/packages/ui/src/features/code-review/components/PatchedFileDiff.tsx +++ b/packages/ui/src/features/code-review/components/PatchedFileDiff.tsx @@ -16,6 +16,7 @@ interface PatchedFileDiffProps { externalUrl?: string; prUrl?: string | null; commentThreads?: Map; + viewedKey?: string; } export function PatchedFileDiff({ @@ -28,6 +29,7 @@ export function PatchedFileDiff({ externalUrl, prUrl, commentThreads, + viewedKey, }: PatchedFileDiffProps) { const fileDiff = useMemo((): FileDiffMetadata | undefined => { if (!file.patch) return undefined; @@ -56,6 +58,7 @@ export function PatchedFileDiff({ collapsed={collapsed} onToggle={onToggle} externalUrl={externalUrl} + viewedKey={viewedKey} /> ); } @@ -72,6 +75,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 caacf02bd4..9d7155d2b5 100644 --- a/packages/ui/src/features/code-review/components/ReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/ReviewPage.tsx @@ -134,7 +134,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 ?? "")), @@ -186,6 +188,8 @@ export function ReviewPage({ task }: ReviewPageProps) { expandAll={expandAll} collapseAll={collapseAll} uncollapseFile={uncollapseFile} + viewedFiles={viewedFiles} + toggleViewed={toggleViewed} refetch={refetch} hasStagedFiles={hasStagedFiles} stagedParsedFiles={stagedParsedFiles} @@ -218,6 +222,8 @@ function LocalReviewContent({ expandAll, collapseAll, uncollapseFile, + viewedFiles, + toggleViewed, refetch, hasStagedFiles, stagedParsedFiles, @@ -246,6 +252,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"]; @@ -357,6 +365,8 @@ function LocalReviewContent({ defaultBranch={defaultBranch} items={items} itemIndexByFilePath={itemIndexByFilePath} + viewedFiles={viewedFiles} + onToggleViewed={toggleViewed} /> ); } @@ -401,7 +411,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( () => @@ -444,6 +454,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 aa5b6e8b5f..34db2bee58 100644 --- a/packages/ui/src/features/code-review/components/ReviewRows.tsx +++ b/packages/ui/src/features/code-review/components/ReviewRows.tsx @@ -64,9 +64,10 @@ export const PatchRow = memo(function PatchRow({ collapsed={collapsed} onToggle={onToggle} onOpenFile={onOpenFile} + viewedKey={itemKey} /> ), - [collapsed, onToggle, onOpenFile], + [collapsed, onToggle, onOpenFile, itemKey], ); return ( ); }); @@ -152,6 +154,7 @@ export const RemoteRow = memo(function RemoteRow({ onToggle={onToggle} commentThreads={commentThreads} externalUrl={externalUrl} + viewedKey={file.path} /> ); }); @@ -163,6 +166,7 @@ function UntrackedFileDiff({ options, collapsed, onToggle, + viewedKey, }: { file: ChangedFile; repoPath: string; @@ -170,6 +174,7 @@ function UntrackedFileDiff({ options: DiffOptions; collapsed: boolean; onToggle: () => void; + viewedKey?: string; }) { const [containerRef, inView] = useInView({ rootMargin: REVIEW_PREFETCH_ROOT_MARGIN, @@ -211,6 +216,7 @@ function UntrackedFileDiff({ reason="line-limit" collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} /> ); } @@ -231,6 +237,7 @@ function UntrackedFileDiff({ fileDiff={fd} collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} /> )} /> @@ -242,6 +249,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 c260b1a358..4ba7d3d6fd 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -1,4 +1,9 @@ -import { ArrowSquareOut, CaretDown } from "@phosphor-icons/react"; +import { + ArrowSquareOut, + CaretDown, + CheckSquare, + Square, +} from "@phosphor-icons/react"; import type { FileDiffMetadata } from "@pierre/diffs/react"; import type { ResolvedDiffSource } from "@posthog/core/code-review/resolveDiffSource"; import { @@ -13,6 +18,8 @@ import { FileIcon } from "../../primitives/FileIcon"; 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 { @@ -46,6 +53,7 @@ function useDiffOptions() { export function useReviewState( changedFiles: ChangedFile[], allPaths: string[], + taskId: string, ) { const diffOptions = useDiffOptions(); @@ -55,8 +63,43 @@ export function useReviewState( ); const collapseState = useCollapseState(allPaths); + const viewedState = useViewedState(taskId, collapseState.setFileCollapsed); + + 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], + ); - return { diffOptions, linesAdded, linesRemoved, ...collapseState }; + // 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[]) { @@ -82,6 +125,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( @@ -93,6 +149,7 @@ function useCollapseState(filePaths: string[]) { collapsedFiles, toggleFile, uncollapseFile, + setFileCollapsed, expandAll, collapseAll, }; @@ -107,6 +164,8 @@ export interface ReviewShellProps { isEmpty: boolean; items: ReviewListItem[]; itemIndexByFilePath: Map; + viewedFiles: Set; + onToggleViewed: (key: string) => void; onUncollapseFile?: (filePath: string) => void; allExpanded: boolean; onExpandAll: () => void; @@ -132,6 +191,7 @@ export function FileHeaderRow({ collapsed, onToggle, trailing, + viewedKey, }: { dirPath: string; fileName: string; @@ -140,6 +200,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 ( + ); } @@ -184,11 +274,13 @@ export function DiffFileHeader({ collapsed, onToggle, onOpenFile, + viewedKey, }: { fileDiff: FileDiffMetadata; collapsed: boolean; onToggle: () => void; onOpenFile?: () => void; + viewedKey?: string; }) { const fullPath = fileDiff.prevName && fileDiff.prevName !== fileDiff.name @@ -205,6 +297,7 @@ export function DiffFileHeader({ deletions={deletions} collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} trailing={ onOpenFile && ( {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 25fce42bcc41a1c5510ab98e7428827dca8d75af Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 18 Jun 2026 15:17:40 -0400 Subject: [PATCH 03/10] 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 9d7155d2b5..4068ba9949 100644 --- a/packages/ui/src/features/code-review/components/ReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/ReviewPage.tsx @@ -134,7 +134,7 @@ export function ReviewPage({ task }: ReviewPageProps) { expandAll, collapseAll, uncollapseFile, - viewedFiles, + viewedRecord, toggleViewed, } = useReviewState(changedFiles, allPaths, taskId); @@ -188,7 +188,7 @@ export function ReviewPage({ task }: ReviewPageProps) { expandAll={expandAll} collapseAll={collapseAll} uncollapseFile={uncollapseFile} - viewedFiles={viewedFiles} + viewedRecord={viewedRecord} toggleViewed={toggleViewed} refetch={refetch} hasStagedFiles={hasStagedFiles} @@ -222,7 +222,7 @@ function LocalReviewContent({ expandAll, collapseAll, uncollapseFile, - viewedFiles, + viewedRecord, toggleViewed, refetch, hasStagedFiles, @@ -252,8 +252,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"]; @@ -365,7 +365,7 @@ function LocalReviewContent({ defaultBranch={defaultBranch} items={items} itemIndexByFilePath={itemIndexByFilePath} - viewedFiles={viewedFiles} + viewedRecord={viewedRecord} onToggleViewed={toggleViewed} /> ); @@ -454,7 +454,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 3300530329..269aa9ceb9 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; @@ -50,6 +80,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[]) { @@ -164,8 +159,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; @@ -180,6 +175,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; } @@ -203,7 +200,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 cde14c9118bb4dfbed8658678c5fd8fd33d3640e Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 18 Jun 2026 15:28:22 -0400 Subject: [PATCH 04/10] 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, 45 insertions(+), 6 deletions(-) diff --git a/packages/core/src/archive/archiveOrchestration.test.ts b/packages/core/src/archive/archiveOrchestration.test.ts index f95370da1c..334d330d6a 100644 --- a/packages/core/src/archive/archiveOrchestration.test.ts +++ b/packages/core/src/archive/archiveOrchestration.test.ts @@ -30,6 +30,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), @@ -60,10 +61,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 a8d8406c41..0cb59c20a9 100644 --- a/packages/core/src/archive/archiveOrchestration.ts +++ b/packages/core/src/archive/archiveOrchestration.ts @@ -40,6 +40,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; } @@ -101,6 +102,9 @@ export async function archiveTask( try { await deps.disconnectFromTask(taskId); await deps.archive(taskId); + // Read state is per-task review convenience; an archived task won't be + // re-reviewed, so drop it once the archive is confirmed. + 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 fabafdcc7b..8c2b08bb80 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"; @@ -138,6 +139,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 e06d3a415a19d6d278c3c12fdf06ade32065243a Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 18 Jun 2026 15:30:17 -0400 Subject: [PATCH 05/10] 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 fdf78bb5bcb9273318dd810e22076ae12b43409f Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 18 Jun 2026 15:31:34 -0400 Subject: [PATCH 06/10] 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 137fceb3328cd86de38eed5e70ab08ae99ddb9b6 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 18 Jun 2026 15:42:16 -0400 Subject: [PATCH 07/10] 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 269aa9ceb9..10aae5f2bb 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 297c8e4c2cddeaa84ec5742613eb040f452938db Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 18 Jun 2026 15:44:54 -0400 Subject: [PATCH 08/10] 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 8c2b08bb80..5d16bd5f10 100644 --- a/packages/ui/src/features/archive/useArchiveTask.ts +++ b/packages/ui/src/features/archive/useArchiveTask.ts @@ -140,7 +140,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 f447bb1dff10beb724e66647b00c04dc573762b6 Mon Sep 17 00:00:00 2001 From: Matt Pua Date: Thu, 18 Jun 2026 15:51:51 -0400 Subject: [PATCH 09/10] 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 10aae5f2bb..6641e8378d 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 a697536e14..a023b16a74 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -244,6 +244,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; @@ -252,8 +261,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 (