diff --git a/packages/core/src/archive/archiveOrchestration.test.ts b/packages/core/src/archive/archiveOrchestration.test.ts index 0f73a1e780..08ae2b9e37 100644 --- a/packages/core/src/archive/archiveOrchestration.test.ts +++ b/packages/core/src/archive/archiveOrchestration.test.ts @@ -29,6 +29,7 @@ class Harness { stopCloudRun: vi.fn().mockResolvedValue(true), disconnectFromTask: vi.fn().mockResolvedValue(undefined), archive: vi.fn().mockResolvedValue(undefined), + clearViewedState: vi.fn(), logError: vi.fn(), cache: { cancelPathFilter: vi.fn().mockResolvedValue(undefined), @@ -59,10 +60,19 @@ describe("archiveTask", () => { expect(harness.deps.archive).toHaveBeenCalledWith(TASK_ID); expect(harness.deps.disconnectFromTask).toHaveBeenCalledWith(TASK_ID); + expect(harness.deps.clearViewedState).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.clearViewedState).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 19d7801c1a..4da7a7690a 100644 --- a/packages/core/src/archive/archiveOrchestration.ts +++ b/packages/core/src/archive/archiveOrchestration.ts @@ -39,6 +39,7 @@ export interface ArchiveOrchestrationDeps { stopCloudRun(taskId: string, runId?: string): Promise; disconnectFromTask(taskId: string): Promise; archive(taskId: string): Promise; + clearViewedState(taskId: string): void; logError(message: string, error: unknown): void; cache: ArchiveCacheWriter; } @@ -103,9 +104,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.clearViewedState(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/core/src/git/router-schemas.ts b/packages/core/src/git/router-schemas.ts index 81c275f171..99273934ea 100644 --- a/packages/core/src/git/router-schemas.ts +++ b/packages/core/src/git/router-schemas.ts @@ -42,6 +42,7 @@ export const changedFileSchema = z.object({ linesRemoved: z.number().optional(), staged: z.boolean().optional(), patch: z.string().optional(), + sha: z.string().optional(), }); export type ChangedFile = z.infer; diff --git a/packages/shared/src/domain-types.ts b/packages/shared/src/domain-types.ts index 5e46d7f947..97622d89ab 100644 --- a/packages/shared/src/domain-types.ts +++ b/packages/shared/src/domain-types.ts @@ -364,6 +364,7 @@ export interface ChangedFile { linesRemoved?: number; staged?: boolean; patch?: string; // Unified diff patch from GitHub API + sha?: string; } // External apps detection types diff --git a/packages/ui/src/features/archive/useArchiveTask.ts b/packages/ui/src/features/archive/useArchiveTask.ts index 2f8000d4e5..a83828320d 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"; @@ -127,6 +128,8 @@ function makeOrchestrationDeps( ), archive: (taskId) => hostClient.archive.archive.mutate({ taskId }).then(() => undefined), + clearViewedState: (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/CloudReviewPage.tsx b/packages/ui/src/features/code-review/components/CloudReviewPage.tsx index 5f92127f31..fc45568bd8 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,19 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { expandAll, collapseAll, uncollapseFile, - } = useReviewState(reviewFiles, allPaths); + collapseFiles, + viewedRecord, + toggleViewed, + } = useReviewState(reviewFiles, allPaths, taskId); + + const currentSignatures = useMemo(() => { + const map = new Map(); + for (const f of reviewFiles) { + const signature = changedFileSignature(f); + if (signature) map.set(f.path, signature); + } + return map; + }, [reviewFiles]); const toolCallFallbacks = useMemo( () => @@ -81,6 +94,7 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { commentThreads={showReviewComments ? commentThreads : undefined} fallback={toolCallFallbacks?.get(file.path) ?? null} externalUrl={githubFileUrl} + viewedKey={file.path} /> ), }; @@ -130,8 +144,12 @@ export function CloudReviewPage({ task }: CloudReviewPageProps) { onExpandAll={expandAll} onCollapseAll={collapseAll} onUncollapseFile={uncollapseFile} + onCollapseFiles={collapseFiles} items={items} itemIndexByFilePath={itemIndexByFilePath} + currentSignatures={currentSignatures} + viewedRecord={viewedRecord} + 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 93b987636a..c8cc4eca00 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; /** Extra controls in the file header row (e.g. a "Viewed" toggle). */ headerTrailing?: ReactNode; } @@ -31,6 +32,7 @@ export function PatchedFileDiff({ externalUrl, prUrl, commentThreads, + viewedKey, headerTrailing, }: PatchedFileDiffProps) { const fileDiff = useMemo((): FileDiffMetadata | undefined => { @@ -64,6 +66,7 @@ export function PatchedFileDiff({ collapsed={collapsed} onToggle={onToggle} externalUrl={externalUrl} + viewedKey={viewedKey} commentCount={commentCount} headerTrailing={headerTrailing} /> @@ -80,6 +83,7 @@ export function PatchedFileDiff({ collapsed={collapsed} onToggle={onToggle} externalUrl={externalUrl} + viewedKey={viewedKey} commentCount={commentCount} headerTrailing={headerTrailing} /> @@ -98,6 +102,7 @@ export function PatchedFileDiff({ fileDiff={fd} collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} commentCount={commentCount} trailing={headerTrailing} /> diff --git a/packages/ui/src/features/code-review/components/ReviewPage.tsx b/packages/ui/src/features/code-review/components/ReviewPage.tsx index 373f311798..d42685815c 100644 --- a/packages/ui/src/features/code-review/components/ReviewPage.tsx +++ b/packages/ui/src/features/code-review/components/ReviewPage.tsx @@ -33,6 +33,8 @@ import { buildPatchReviewItems, buildRemoteReviewItems, buildUntrackedReviewItems, + changedFileSignature, + patchFileSignature, } from "./reviewItemBuilders"; const EMPTY_CHANGED_FILES: ChangedFile[] = []; @@ -138,7 +140,10 @@ export function ReviewPage({ task }: ReviewPageProps) { expandAll, collapseAll, uncollapseFile, - } = useReviewState(changedFiles, allPaths); + collapseFiles, + viewedRecord, + toggleViewed, + } = useReviewState(changedFiles, allPaths, taskId); const stagedPathSet = useMemo( () => new Set(stagedParsedFiles.map((f) => f.name ?? f.prevName ?? "")), @@ -191,6 +196,9 @@ export function ReviewPage({ task }: ReviewPageProps) { expandAll={expandAll} collapseAll={collapseAll} uncollapseFile={uncollapseFile} + collapseFiles={collapseFiles} + viewedRecord={viewedRecord} + toggleViewed={toggleViewed} refetch={refetch} hasStagedFiles={hasStagedFiles} stagedParsedFiles={stagedParsedFiles} @@ -224,6 +232,9 @@ function LocalReviewContent({ expandAll, collapseAll, uncollapseFile, + collapseFiles, + viewedRecord, + toggleViewed, refetch, hasStagedFiles, stagedParsedFiles, @@ -253,6 +264,9 @@ function LocalReviewContent({ expandAll: () => void; collapseAll: () => void; uncollapseFile: (filePath: string) => void; + collapseFiles: (keys: string[]) => void; + viewedRecord: Record; + toggleViewed: (key: string, sig: string | null) => void; refetch: () => void; hasStagedFiles: boolean; stagedParsedFiles: ReturnType[number]["files"]; @@ -295,6 +309,27 @@ function LocalReviewContent({ [filesByKey, stageToggle], ); + const currentSignatures = useMemo(() => { + const map = new Map(); + for (const f of stagedParsedFiles) { + map.set( + makeFileKey(true, f.name ?? f.prevName ?? ""), + patchFileSignature(f), + ); + } + for (const f of unstagedParsedFiles) { + map.set( + makeFileKey(false, f.name ?? f.prevName ?? ""), + patchFileSignature(f), + ); + } + for (const f of untrackedFiles) { + const signature = changedFileSignature(f); + if (signature) map.set(makeFileKey(f.staged, f.path), signature); + } + return map; + }, [stagedParsedFiles, unstagedParsedFiles, untrackedFiles]); + const items = useMemo(() => { const reviewItems: ReviewListItem[] = []; @@ -393,6 +428,7 @@ function LocalReviewContent({ onExpandAll={expandAll} onCollapseAll={collapseAll} onUncollapseFile={uncollapseFile} + onCollapseFiles={collapseFiles} onRefresh={refetch} onDiscardAll={totalFileCount > 0 ? discardAllChanges : undefined} effectiveSource={effectiveSource} @@ -401,6 +437,9 @@ function LocalReviewContent({ defaultBranch={defaultBranch} items={items} itemIndexByFilePath={itemIndexByFilePath} + currentSignatures={currentSignatures} + viewedRecord={viewedRecord} + onToggleViewed={toggleViewed} /> ); } @@ -455,7 +494,16 @@ 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 currentSignatures = useMemo(() => { + const map = new Map(); + for (const f of files) { + const signature = changedFileSignature(f); + if (signature) map.set(f.path, signature); + } + return map; + }, [files]); const items = useMemo( () => @@ -492,6 +540,7 @@ function RemoteReviewPage({ onExpandAll={reviewState.expandAll} onCollapseAll={reviewState.collapseAll} onUncollapseFile={reviewState.uncollapseFile} + onCollapseFiles={reviewState.collapseFiles} onRefresh={onRefresh} effectiveSource={effectiveSource} branchSourceAvailable={branchSourceAvailable} @@ -499,6 +548,9 @@ function RemoteReviewPage({ defaultBranch={defaultBranch} items={items} itemIndexByFilePath={itemIndexByFilePath} + currentSignatures={currentSignatures} + viewedRecord={reviewState.viewedRecord} + 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 a351656134..3e03271bfc 100644 --- a/packages/ui/src/features/code-review/components/ReviewShell.tsx +++ b/packages/ui/src/features/code-review/components/ReviewShell.tsx @@ -1,8 +1,11 @@ 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, 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 +15,13 @@ import { useReviewDraftsStore } from "../reviewDraftsStore"; import { REVIEW_HOST, type ReviewHost } from "../reviewHost"; import { useReviewNavigationStore } from "../reviewNavigationStore"; import type { ReviewListItem, ReviewShellProps } from "../reviewShellParts"; +import { + findActiveScrollKey, + findRenderedScrollAnchor, + isFileViewed, +} from "../reviewShellParts"; +import { ReviewViewedContext } from "../reviewViewedContext"; +import { useReviewViewedStore } from "../reviewViewedStore"; import { PendingReviewBar } from "./PendingReviewBar"; import { ReviewToolbar } from "./ReviewToolbar"; @@ -103,7 +113,11 @@ export function ReviewShell({ isEmpty, items, itemIndexByFilePath, + currentSignatures, + viewedRecord, + onToggleViewed, onUncollapseFile, + onCollapseFiles, allExpanded, onExpandAll, onCollapseAll, @@ -117,6 +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 pendingNavigationRef = useRef(null); + const navigationFrameRef = useRef(null); const workerFactory = useCallback( () => reviewHost.diffWorkerFactory(), @@ -128,6 +146,56 @@ export function ReviewShell({ ); const isExpanded = reviewMode === "expanded"; + const viewedCount = useMemo(() => { + let count = 0; + for (const [key, sig] of currentSignatures) { + if (isFileViewed(viewedRecord[key], sig)) count++; + } + return count; + }, [currentSignatures, viewedRecord]); + + // 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; + if (currentSignatures.size === 0) return; + seededTaskRef.current = taskId; + const viewedKeys: string[] = []; + for (const [key, sig] of currentSignatures) { + if (isFileViewed(viewedRecord[key], sig)) viewedKeys.push(key); + } + if (viewedKeys.length > 0) onCollapseFiles(viewedKeys); + }, [taskId, currentSignatures, viewedRecord, onCollapseFiles]); + + const clearTasks = useReviewViewedStore((s) => s.clearTasks); + + const archivedTaskIds = useArchivedTaskIds(); + useEffect(() => { + const prunable = [...archivedTaskIds].filter((id) => id !== taskId); + if (prunable.length > 0) clearTasks(prunable); + }, [archivedTaskIds, clearTasks, taskId]); + + const cloudPrUrl = useCloudPrUrl(taskId); + const { prState } = useTaskPrStatus({ + id: taskId, + cloudPrUrl, + taskRunEnvironment: task.latest_run?.environment, + }); + useEffect(() => { + if (prState === "merged") clearTasks([taskId]); + }, [prState, taskId, clearTasks]); + + const viewedContextValue = useMemo( + () => ({ + viewedRecord, + currentSignatures, + toggleViewed: onToggleViewed, + }), + [viewedRecord, currentSignatures, onToggleViewed], + ); + const scrollRequest = useReviewNavigationStore( (s) => s.scrollRequests[taskId] ?? null, ); @@ -141,6 +209,9 @@ export function ReviewShell({ useEffect(() => { return () => { + if (navigationFrameRef.current !== null) { + cancelAnimationFrame(navigationFrameRef.current); + } clearTask(taskId); useReviewDraftsStore.getState().clearDrafts(taskId); }; @@ -151,35 +222,63 @@ export function ReviewShell({ const targetIndex = itemIndexByFilePath.get(scrollRequest); if (targetIndex === undefined) return; - onUncollapseFile?.(scrollRequest); - requestAnimationFrame(() => { + const currentSignature = currentSignatures.get(scrollRequest); + const viewed = + currentSignature !== undefined && + isFileViewed(viewedRecord[scrollRequest], currentSignature); + if (navigationFrameRef.current !== null) { + cancelAnimationFrame(navigationFrameRef.current); + } + pendingNavigationRef.current = scrollRequest; + if (!viewed) onUncollapseFile?.(scrollRequest); + + const scrollToAnchor = (remainingAttempts: number) => { listRef.current?.scrollToIndex(targetIndex, { align: "start" }); - 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, itemIndexByFilePath, onUncollapseFile, scrollRequest, setActiveFilePath, taskId, + viewedRecord, ]); - const lastActiveRef = useRef(null); - const handleScroll = useCallback( - (offset: number) => { - 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 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) => ( @@ -220,54 +319,69 @@ 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/components/ReviewToolbar.tsx b/packages/ui/src/features/code-review/components/ReviewToolbar.tsx index 3c07915eb5..f52d39e0d6 100644 --- a/packages/ui/src/features/code-review/components/ReviewToolbar.tsx +++ b/packages/ui/src/features/code-review/components/ReviewToolbar.tsx @@ -22,6 +22,7 @@ import { DiffSourceSelector } from "./DiffSourceSelector"; interface ReviewToolbarProps { taskId: string; fileCount: number; + viewedCount: number; linesAdded: number; linesRemoved: number; allExpanded: boolean; @@ -38,6 +39,7 @@ interface ReviewToolbarProps { export const ReviewToolbar = memo(function ReviewToolbar({ taskId, fileCount, + viewedCount, allExpanded, onExpandAll, onCollapseAll, @@ -79,6 +81,11 @@ export const ReviewToolbar = memo(function ReviewToolbar({ {fileCount} file{fileCount !== 1 ? "s" : ""} changed + {fileCount > 0 && ( + + {viewedCount}/{fileCount} viewed + + )} {effectiveSource && ( ): 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("uses the blob sha when no patch is available", () => { + expect(changedFileSignature(changedFile({ sha: "abc" }))).toBe( + "modified:abc", + ); + expect(changedFileSignature(changedFile({ sha: "def" }))).toBe( + "modified:def", + ); + }); + + it("returns no signature without patch content or a blob sha", () => { + expect( + changedFileSignature(changedFile({ linesAdded: 1, linesRemoved: 2 })), + ).toBeNull(); + }); +}); + +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 b476b2f021..5e569f4b09 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,24 @@ import type { ReviewListItem } from "../reviewShellParts"; import type { DiffOptions } from "../types"; import { PatchRow, RemoteRow, UntrackedRow } from "./ReviewRows"; +export function changedFileSignature(file: ChangedFile): string | null { + if (file.patch) return contentHash(file.patch); + if (file.sha) return `${file.status}:${file.sha}`; + return null; +} + +export function patchFileSignature( + fileDiff: ReturnType[number]["files"][number], +): string { + // 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. + return fileDiff.newObjectId || fileDiff.prevObjectId + ? `${fileDiff.prevObjectId ?? ""}:${fileDiff.newObjectId ?? ""}` + : contentHash(JSON.stringify(fileDiff.hunks ?? [])); +} + interface BuildPatchReviewItemsArgs { files: ReturnType[number]["files"]; staged?: boolean; diff --git a/packages/ui/src/features/code-review/reviewShellParts.test.tsx b/packages/ui/src/features/code-review/reviewShellParts.test.tsx index a07d37b55b..c51b63a2f9 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.test.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.test.tsx @@ -14,7 +14,12 @@ vi.mock("../../primitives/FileIcon", () => ({ FileIcon: () => , })); -import { DeferredDiffPlaceholder, DiffFileHeader } from "./reviewShellParts"; +import { + DeferredDiffPlaceholder, + DiffFileHeader, + findActiveScrollKey, + findRenderedScrollAnchor, +} from "./reviewShellParts"; type FileDiffMetadata = import("@pierre/diffs/react").FileDiffMetadata; @@ -107,3 +112,52 @@ 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 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"); + 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"); + }); + + 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 ae283f7e92..fa1327522f 100644 --- a/packages/ui/src/features/code-review/reviewShellParts.tsx +++ b/packages/ui/src/features/code-review/reviewShellParts.tsx @@ -3,8 +3,10 @@ import { ArrowSquareOut, CaretDown, ChatCircle, + CheckSquare, Minus, Plus, + Square, } from "@phosphor-icons/react"; import type { FileDiffMetadata } from "@pierre/diffs/react"; import type { ResolvedDiffSource } from "@posthog/core/code-review/resolveDiffSource"; @@ -22,6 +24,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 { @@ -30,6 +34,36 @@ 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; + let activeScrollKey: string | null = null; + for (const anchor of root.querySelectorAll( + SCROLL_ANCHOR_SELECTOR, + )) { + const scrollKey = anchor.dataset.scrollKey; + if (!scrollKey) continue; + if (anchor.getBoundingClientRect().top <= rootTop + 1) { + activeScrollKey = scrollKey; + continue; + } + return activeScrollKey ?? scrollKey; + } + return activeScrollKey; +} export function useDiffOptions() { const viewMode = useDiffViewerStore((s) => s.viewMode); @@ -55,6 +89,7 @@ export function useDiffOptions() { export function useReviewState( changedFiles: ChangedFile[], allPaths: string[], + taskId: string, ) { const diffOptions = useDiffOptions(); @@ -64,8 +99,36 @@ export function useReviewState( ); const collapseState = useCollapseState(allPaths); + const viewedState = useViewedState(taskId, collapseState.setFileCollapsed); + + return { + diffOptions, + linesAdded, + linesRemoved, + ...collapseState, + ...viewedState, + }; +} + +const EMPTY_VIEWED_RECORD: Record = {}; + +function useViewedState( + taskId: string, + setFileCollapsed: (filePath: string, collapsed: boolean) => void, +) { + const viewedRecord = + useReviewViewedStore((s) => s.viewed[taskId]) ?? EMPTY_VIEWED_RECORD; + const setViewed = useReviewViewedStore((s) => s.setViewed); + + const toggleViewed = useCallback( + (key: string, nextSig: string | null) => { + setViewed(taskId, key, nextSig); + setFileCollapsed(key, nextSig !== null); + }, + [taskId, setViewed, setFileCollapsed], + ); - return { diffOptions, linesAdded, linesRemoved, ...collapseState }; + return { viewedRecord, toggleViewed }; } function useCollapseState(filePaths: string[]) { @@ -91,6 +154,33 @@ 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 collapseFiles = useCallback((keys: Iterable) => { + setCollapsedFiles((prev) => { + let changed = false; + const next = new Set(prev); + for (const key of keys) { + if (!next.has(key)) { + next.add(key); + changed = true; + } + } + return changed ? next : prev; + }); + }, []); + const expandAll = useCallback(() => setCollapsedFiles(new Set()), []); const collapseAll = useCallback( @@ -102,6 +192,8 @@ function useCollapseState(filePaths: string[]) { collapsedFiles, toggleFile, uncollapseFile, + setFileCollapsed, + collapseFiles, expandAll, collapseAll, }; @@ -116,7 +208,11 @@ export interface ReviewShellProps { isEmpty: boolean; items: ReviewListItem[]; itemIndexByFilePath: Map; + currentSignatures: Map; + viewedRecord: Record; + onToggleViewed: (key: string, sig: string | null) => void; onUncollapseFile?: (filePath: string) => void; + onCollapseFiles: (keys: string[]) => void; allExpanded: boolean; onExpandAll: () => void; onCollapseAll: () => void; @@ -143,6 +239,7 @@ export function FileHeaderRow({ onToggle, commentCount, trailing, + viewedKey, }: { dirPath: string; fileName: string; @@ -152,44 +249,98 @@ export function FileHeaderRow({ onToggle: () => void; commentCount?: number; trailing?: ReactNode; + viewedKey?: string; }) { return ( - + {trailing} + {viewedKey !== undefined && } + + ); +} + +export function isFileViewed( + storedSig: string | undefined, + currentSig: string, +): boolean { + return storedSig === currentSig; +} + +function ViewedCheckbox({ viewedKey }: { viewedKey: string }) { + const ctx = useReviewViewedContext(); + if (!ctx) return null; + + const current = ctx.currentSignatures.get(viewedKey); + if (current === undefined) return null; + + const stored = ctx.viewedRecord[viewedKey]; + const viewed = isFileViewed(stored, current); + const changed = stored !== undefined && !viewed; + let title = "Mark as viewed"; + if (changed) { + title = "Changed since you viewed it: click to mark as viewed again"; + } else if (viewed) { + title = "Mark as not viewed"; + } + + return ( + ); } @@ -217,6 +368,7 @@ export function DiffFileHeader({ onDiscard, onStage, staged, + viewedKey, commentCount, trailing, }: { @@ -227,6 +379,7 @@ export function DiffFileHeader({ onDiscard?: () => void; onStage?: () => void; staged?: boolean; + viewedKey?: string; commentCount?: number; /** Extra controls rendered after the action buttons (e.g. a "Viewed" toggle). */ trailing?: ReactNode; @@ -246,6 +399,7 @@ export function DiffFileHeader({ deletions={deletions} collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} commentCount={commentCount} trailing={ (onStage || onDiscard || onOpenFile || trailing) && ( @@ -309,6 +463,7 @@ export function DeferredDiffPlaceholder({ onToggle, onShow, externalUrl, + viewedKey, commentCount, headerTrailing, }: { @@ -320,6 +475,7 @@ export function DeferredDiffPlaceholder({ onToggle: () => void; onShow?: () => void; externalUrl?: string; + viewedKey?: string; commentCount?: number; /** Extra controls in the header row (e.g. a "Viewed" toggle). */ headerTrailing?: ReactNode; @@ -335,6 +491,7 @@ export function DeferredDiffPlaceholder({ deletions={linesRemoved} collapsed={collapsed} onToggle={onToggle} + viewedKey={viewedKey} commentCount={commentCount} trailing={ headerTrailing && ( 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..888017fcdc --- /dev/null +++ b/packages/ui/src/features/code-review/reviewViewedContext.ts @@ -0,0 +1,14 @@ +import { createContext, useContext } from "react"; + +export interface ReviewViewedContextValue { + viewedRecord: Record; + currentSignatures: Map; + toggleViewed: (key: string, sig: string | null) => void; +} + +export const ReviewViewedContext = + createContext(null); + +export function useReviewViewedContext(): ReviewViewedContextValue | null { + return useContext(ReviewViewedContext); +} diff --git a/packages/ui/src/features/code-review/reviewViewedStore.test.ts b/packages/ui/src/features/code-review/reviewViewedStore.test.ts new file mode 100644 index 0000000000..554eea5df0 --- /dev/null +++ b/packages/ui/src/features/code-review/reviewViewedStore.test.ts @@ -0,0 +1,42 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { useReviewViewedStore } from "./reviewViewedStore"; + +const { setViewed, clearTasks } = useReviewViewedStore.getState(); +const viewed = () => useReviewViewedStore.getState().viewed; + +describe("reviewViewedStore", () => { + beforeEach(() => useReviewViewedStore.setState({ viewed: {} })); + + it("marks a file read at its signature", () => { + setViewed("t1", "a.ts", "sig1"); + expect(viewed().t1).toEqual({ "a.ts": "sig1" }); + }); + + it("unmarks a file and drops the task once it has no read files", () => { + setViewed("t1", "a.ts", "sig1"); + setViewed("t1", "a.ts", null); + expect(viewed().t1).toBeUndefined(); + }); + + it("keeps other read files when unmarking one", () => { + setViewed("t1", "a.ts", "s"); + setViewed("t1", "b.ts", "s"); + setViewed("t1", "a.ts", null); + expect(viewed().t1).toEqual({ "b.ts": "s" }); + }); + + it("clearTasks removes the given tasks only", () => { + setViewed("t1", "a", "s"); + setViewed("t2", "a", "s"); + setViewed("t3", "a", "s"); + clearTasks(["t1", "t3"]); + expect(Object.keys(viewed())).toEqual(["t2"]); + }); + + it("clearTasks is a no-op (same reference) when nothing matches", () => { + setViewed("t1", "a", "s"); + const before = viewed(); + clearTasks(["unknown"]); + expect(viewed()).toBe(before); + }); +}); 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..eb63bdc45d --- /dev/null +++ b/packages/ui/src/features/code-review/reviewViewedStore.ts @@ -0,0 +1,51 @@ +import { create } from "zustand"; +import { persist } from "zustand/middleware"; + +interface ReviewViewedStoreState { + viewed: Record>; +} + +interface ReviewViewedStoreActions { + setViewed: (taskId: string, key: string, sig: string | null) => void; + clearTasks: (taskIds: Iterable) => void; +} + +type ReviewViewedStore = ReviewViewedStoreState & ReviewViewedStoreActions; + +export const useReviewViewedStore = create()( + persist( + (set) => ({ + viewed: {}, + setViewed: (taskId, key, sig) => + set((state) => { + const taskViewed = { ...(state.viewed[taskId] ?? {}) }; + if (sig === null) delete taskViewed[key]; + else taskViewed[key] = sig; + const next = { ...state.viewed }; + if (Object.keys(taskViewed).length > 0) next[taskId] = taskViewed; + else delete next[taskId]; + return { viewed: next }; + }), + clearTasks: (taskIds) => + set((state) => { + let changed = false; + const next = { ...state.viewed }; + for (const id of taskIds) { + if (id in next) { + delete next[id]; + changed = true; + } + } + return changed ? { viewed: next } : state; + }), + }), + { + name: "review-viewed-storage", + version: 1, + migrate: (persisted, version) => { + if (version < 1) return { viewed: {} }; + return persisted as ReviewViewedStoreState; + }, + }, + ), +); 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 && ( + + )} ); diff --git a/packages/workspace-server/src/services/git/schemas.ts b/packages/workspace-server/src/services/git/schemas.ts index 722663edbf..9cfd15795a 100644 --- a/packages/workspace-server/src/services/git/schemas.ts +++ b/packages/workspace-server/src/services/git/schemas.ts @@ -30,6 +30,7 @@ export const changedFileSchema = z.object({ linesRemoved: z.number().optional(), staged: z.boolean().optional(), patch: z.string().optional(), + sha: z.string().optional(), }); export type ChangedFile = z.infer; diff --git a/packages/workspace-server/src/services/git/service.ts b/packages/workspace-server/src/services/git/service.ts index 468e557b34..361905139f 100644 --- a/packages/workspace-server/src/services/git/service.ts +++ b/packages/workspace-server/src/services/git/service.ts @@ -1061,6 +1061,7 @@ export class GitService extends TypedEventEmitter { additions: number; deletions: number; patch?: string; + sha?: string; }> >; const files = pages.flat(); @@ -1088,6 +1089,7 @@ export class GitService extends TypedEventEmitter { originalPath: f.previous_filename, linesAdded: f.additions, linesRemoved: f.deletions, + sha: f.sha, patch: f.patch ? toUnifiedDiffPatch(f.patch, f.filename, f.previous_filename, status) : undefined, @@ -1247,6 +1249,7 @@ export class GitService extends TypedEventEmitter { additions: number; deletions: number; patch?: string; + sha?: string; }>; }; const files = response.files; @@ -1276,6 +1279,7 @@ export class GitService extends TypedEventEmitter { originalPath: f.previous_filename, linesAdded: f.additions, linesRemoved: f.deletions, + sha: f.sha, patch: f.patch ? toUnifiedDiffPatch(f.patch, f.filename, f.previous_filename, status) : undefined,