diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx index b06f3d117b9ba..3dceab3fd0166 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.browser.test.tsx @@ -85,7 +85,12 @@ vi.mock(import("./usePullRequestFilesViewed"), async () => { enabled: viewedPaths !== null, isViewed: (path: string) => viewedPaths?.has(path) === true, isStale: () => false, - setViewed: () => {}, + setViewed: (path: string, viewed: boolean) => { + const next = new Set(mocks.viewedPaths); + if (viewed) next.add(path); + else next.delete(path); + deliverViewedPaths(next); + }, truncated: false, error: null, refresh: () => {}, @@ -163,12 +168,12 @@ const detail: PullRequestDetailView = { }; function setup({ paged }: { paged: boolean }) { - window.localStorage.removeItem("t3code.diffHideTestFiles"); __setClientSettingsForTests(DEFAULT_CLIENT_SETTINGS); mocks.paged = paged; mocks.diffCommits = []; mocks.viewedPaths = null; mocks.viewedListeners.clear(); + localStorage.clear(); } const tab = ( @@ -176,12 +181,13 @@ const tab = ( revealRequest: { readonly path: string; readonly nonce: number } | null = null, revealUrl?: string, onRevealSettled?: (url: string) => void, + tabDetail: PullRequestDetailView = detail, ) => ( {}} environmentId={EnvironmentId.make("environment")} - reference={detail} - detail={detail} + reference={tabDetail} + detail={tabDetail} selectedCommitOid={null} onSelectedCommitChange={() => {}} onRefresh={() => {}} @@ -265,7 +271,6 @@ it("pages forward to a file asked for that is not loaded yet, then scrolls to it __setClientSettingsForTests({ ...DEFAULT_CLIENT_SETTINGS, diffFilesCollapsed: true }); using _preferences = { [Symbol.dispose]() { - window.localStorage.removeItem("t3code.diffHideTestFiles"); __setClientSettingsForTests(DEFAULT_CLIENT_SETTINGS); }, }; @@ -288,6 +293,80 @@ it("pages forward to a file asked for that is not loaded yet, then scrolls to it await expectFileHeaderShown(screen, container, "late.test.ts"); }); +it("hides viewed files and shows them again", async () => { + setup({ paged: true }); + mocks.viewedPaths = new Set(["early-0.ts"]); + await using screen = await render(tab(0), { container: pagedContainer() }); + await expect.element(screen.getByText("early-0.ts", { exact: true }).first()).toBeVisible(); + const hideViewed = screen.getByRole("button", { name: "Hide viewed files" }); + + await hideViewed.click(); + + await expect.element(screen.getByText("early-0.ts", { exact: true })).not.toBeInTheDocument(); + await expect.element(screen.getByText("early-2.ts", { exact: true }).first()).toBeVisible(); + await hideViewed.click(); + await expect.element(screen.getByText("early-0.ts", { exact: true }).first()).toBeVisible(); +}); + +it("keeps the Hide viewed files toggle while its saved filter is on and nothing is viewed", async () => { + setup({ paged: true }); + mocks.viewedPaths = new Set(); + localStorage.setItem("t3code.pullRequestHideViewedFiles", "true"); + await using screen = await render(tab(0), { container: pagedContainer() }); + await screen.getByRole("button", { name: "Hide viewed files" }).click(); + + await vi.waitFor(() => + expect(localStorage.getItem("t3code.pullRequestHideViewedFiles")).toBe("false"), + ); +}); + +it("marks a Shift-clicked range viewed after the first file of it was hidden", async () => { + setup({ paged: true }); + mocks.viewedPaths = new Set(["early-0.ts"]); + const container = document.createElement("div"); + container.className = "h-[500px] w-[400px]"; + await using screen = await render(tab(0), { container }); + await screen.getByRole("button", { name: "Hide viewed files" }).click(); + await expect.element(screen.getByText("early-10.ts", { exact: true }).first()).toBeVisible(); + + await screen.getByRole("checkbox", { name: "Viewed" }).first().click(); + await expect.element(screen.getByText("early-10.ts", { exact: true })).not.toBeInTheDocument(); + await screen + .getByRole("checkbox", { name: "Viewed" }) + .nth(1) + .click({ modifiers: ["Shift"] }); + + await expect.element(screen.getByText("early-11.ts", { exact: true })).not.toBeInTheDocument(); +}); + +it("keeps conversations reachable when every file is hidden", async () => { + setup({ paged: false }); + mocks.viewedPaths = new Set(["a.ts"]); + const threadDetail: PullRequestDetailView = { + ...detail, + reviewThreads: [ + { + id: "thread", + path: "a.ts", + line: 1, + side: "right", + isResolved: false, + isOutdated: false, + comments: [], + }, + ], + }; + const container = document.createElement("div"); + container.className = "h-[500px] w-[400px]"; + await using screen = await render(tab(0, null, undefined, undefined, threadDetail), { + container, + }); + await screen.getByRole("button", { name: "Hide viewed files" }).click(); + await expect.element(screen.getByText("All files are hidden.")).toBeVisible(); + + await expect.element(screen.getByText("Conversations not on the current diff")).toBeVisible(); +}); + it("never requests the diff of a selected commit the pull request no longer has", async () => { setup({ paged: false }); const container = document.createElement("div"); diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 68525756e2d99..7617fccae65a1 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -24,6 +24,7 @@ import { ChevronDownIcon, ChevronRightIcon, Columns2Icon, + EyeOffIcon, FlaskConicalOffIcon, FolderTreeIcon, InfoIcon, @@ -149,6 +150,7 @@ type ReviewAnnotation = DiffLineAnnotation; const COMMIT_PAGE_SIZE = 10; const PULL_REQUEST_FILE_TREE_STORAGE_KEY = "t3code.pullRequestFileTreeOpen"; +const PULL_REQUEST_HIDE_VIEWED_FILES_STORAGE_KEY = "t3code.pullRequestHideViewedFiles"; interface DiffSlice { readonly source: PullRequestDiffResult; @@ -401,6 +403,11 @@ function PullRequestCodeTab({ Schema.Boolean, ); const [hideTestFiles, setHideTestFiles] = useHideTestFiles(); + const [hideViewedFiles, setHideViewedFiles] = useLocalStorage( + PULL_REQUEST_HIDE_VIEWED_FILES_STORAGE_KEY, + false, + Schema.Boolean, + ); const [selectedLines, setSelectedLines] = useState<{ id: string; range: SelectedLineRange; @@ -603,15 +610,28 @@ function PullRequestCodeTab({ const testFileCount = loadedFiles.filter((file) => isTestChangedFile(resolveFileDiffPath(file)), ).length; - // A requested file stays visible so a jump to a hidden test still lands. - const files = hideTestFiles + // A filter never hides the file being jumped to or one holding unsent review comments. + const isPinned = (path: string) => + path === revealRequest?.path || + path === linkPath || + path === draft?.path || + pendingComments.some((comment) => comment.path === path); + const reviewFiles = hideTestFiles ? loadedFiles.filter((file) => { const path = resolveFileDiffPath(file); - return path === revealRequest?.path || path === linkPath || !isTestChangedFile(path); + return isPinned(path) || !isTestChangedFile(path); }) : loadedFiles; - const hiddenTestFileCount = loadedFiles.length - files.length; - const filePaths = files.map((file) => resolveFileDiffPath(file)); + const hiddenTestFileCount = loadedFiles.length - reviewFiles.length; + const viewedFileCount = reviewFiles.filter((file) => + isFileViewed(resolveFileDiffPath(file)), + ).length; + const files = hideViewedFiles + ? reviewFiles.filter((file) => { + const path = resolveFileDiffPath(file); + return isPinned(path) || !isFileViewed(path); + }) + : reviewFiles; const omittedFileStats = new Map( loadedSlices.flatMap((slice) => slice.omittedFileStats.map((file) => [file.path, file] as const), @@ -805,11 +825,12 @@ function PullRequestCodeTab({ }; const setFileViewed = (fileKey: string, viewed: boolean, extendRange: boolean) => { - const range = getFileViewedRange( - annotatedFiles, - fileKey, - extendRange ? lastViewedFileKey : null, - ); + // Include files hidden as viewed: the Shift-click anchor is usually one of them. + const rangeFiles = reviewFiles.map((file) => ({ + fileKey: buildFileDiffRenderKey(file), + path: resolveFileDiffPath(file), + })); + const range = getFileViewedRange(rangeFiles, fileKey, extendRange ? lastViewedFileKey : null); setLastViewedFileKey(fileKey); for (const file of range) setViewed(file.path, viewed); setFoldChoices((current) => @@ -1190,10 +1211,10 @@ function PullRequestCodeTab({ {hiddenTestFileCount} test {hiddenTestFileCount === 1 ? "file" : "files"} hidden ) : null} - {filesViewed.enabled && files.length > 0 ? ( + {filesViewed.enabled && reviewFiles.length > 0 ? ( - {filePaths.filter(isFileViewed).length} / {files.length} + {viewedFileCount} / {reviewFiles.length} {viewedFilesStore === "environment" ? `viewed in ${APP_BASE_NAME}` : "viewed"} @@ -1423,6 +1444,26 @@ function PullRequestCodeTab({ ) : null} + {viewedFileCount > 0 || hideViewedFiles ? ( + + setHideViewedFiles(Boolean(pressed))} + /> + } + > + + + + {hideViewedFiles ? "Show viewed files" : "Hide viewed files"} + + + ) : null} {fileKeys.length > 0 ? ( - {commit === null ? `This ${term} has no file changes.` : "This commit has no file changes."} -

, - ); - } - const orphanThreads = detail.reviewThreads.filter((thread) => !placedThreadIds.has(thread.id)); const orphanFiles = new Map(); for (const thread of orphanThreads) { @@ -1554,63 +1587,76 @@ function PullRequestCodeTab({ if (existing) existing.push(thread); else orphanFiles.set(thread.path, [thread]); } + const orphanConversations = + orphanFiles.size > 0 ? ( +
+ +

+ + + {nextCursor === null + ? "Conversations not on the current diff" + : "Conversations not on the diff loaded so far"} + + + + {orphanThreads.length} + + + {orphanThreads.length === 1 + ? "1 conversation" + : `${orphanThreads.length} conversations`} + + +

+ +
+ {[...orphanFiles].map(([path, threads]) => ( +
+ + {path}

} + /> + {path} + +
+ {threads.map((thread) => ( +
+ {thread.line === null ? null : ( +

Line {thread.line}

+ )} + {renderThreadCard(thread)} +
+ ))} +
+
+ ))} +
+ + +
+ ) : null; + + if (items.length === 0 && nextCursor === null) { + const noFilesMessage = + commit === null ? `This ${term} has no file changes.` : "This commit has no file changes."; + return withToolbar( + <> + {orphanConversations} +

+ {loadedFiles.length > 0 ? "All files are hidden." : noFilesMessage} +

+ , + ); + } return (
{toolbar} - {orphanFiles.size > 0 ? ( -
- -

- - - {nextCursor === null - ? "Conversations not on the current diff" - : "Conversations not on the diff loaded so far"} - - - - {orphanThreads.length} - - - {orphanThreads.length === 1 - ? "1 conversation" - : `${orphanThreads.length} conversations`} - - -

- -
- {[...orphanFiles].map(([path, threads]) => ( -
- - {path}

- } - /> - {path} - -
- {threads.map((thread) => ( -
- {thread.line === null ? null : ( -

Line {thread.line}

- )} - {renderThreadCard(thread)} -
- ))} -
-
- ))} -
- - -
- ) : null} + {orphanConversations}