diff --git a/apps/mobile/src/features/pull-requests/PullRequestFiles.tsx b/apps/mobile/src/features/pull-requests/PullRequestFiles.tsx index 7244945878861..613ffdf1b11c6 100644 --- a/apps/mobile/src/features/pull-requests/PullRequestFiles.tsx +++ b/apps/mobile/src/features/pull-requests/PullRequestFiles.tsx @@ -30,6 +30,7 @@ import { } from "./usePullRequestDiff"; import { usePullRequestFileFilter } from "./usePullRequestFileFilter"; import { usePullRequestFilesViewed } from "./usePullRequestFilesViewed"; +import { usePullRequestReviewDraft } from "./usePullRequestReviewDraft"; import { pullRequestFileJumpScope, requestPullRequestFileJump } from "./pullRequestFileJump"; const BREAKDOWN_LABELS = { @@ -54,7 +55,13 @@ function useChangedFiles(params: PullRequestDiffParams) { ).changeRequest; const diff = usePullRequestDiff(params, terminology); const viewed = usePullRequestFilesViewed(params, pr); - const filter = usePullRequestFileFilter(diff.parsed); + const reviewDraft = usePullRequestReviewDraft(params); + const filter = usePullRequestFileFilter( + diff.parsed, + viewed, + undefined, + reviewDraft.draft.comments.map((comment) => comment.path), + ); const parsed = filter.parsed; return { diff, @@ -78,11 +85,14 @@ function describeFiles( { filter, viewed }: ChangedFiles, ) { const noun = (count: number) => (count === 1 ? "file" : "files"); + const hiding = filter.hiddenCount > 0 || filter.hiddenViewedCount > 0; return [ - `${parsed.fileCount} ${filter.hiddenCount ? "visible" : "changed"} ${noun(parsed.fileCount)}`, - viewed.supported - ? `${parsed.files.filter((file) => viewed.isViewed(file.path)).length} viewed` - : null, + `${parsed.fileCount} ${hiding ? "visible" : "changed"} ${noun(parsed.fileCount)}`, + !viewed.supported + ? null + : filter.hideViewedFiles + ? `${filter.hiddenViewedCount} viewed ${noun(filter.hiddenViewedCount)} hidden` + : `${filter.viewedCount} viewed`, filter.hideTestFiles ? `${filter.hiddenCount} test ${noun(filter.hiddenCount)} hidden` : "Test files included", @@ -166,14 +176,26 @@ function FileFilterMenu({ filter }: { readonly filter: ChangedFiles["filter"] }) image: "line.3.horizontal.decrease", state: filter.hideTestFiles ? "on" : "off", }, + ...(filter.canHideViewedFiles + ? [ + { + id: "hide-viewed", + title: "Hide viewed files", + image: "eye.slash", + state: filter.hideViewedFiles ? ("on" as const) : ("off" as const), + }, + ] + : []), ]} - onPressAction={filter.toggle} + onPressAction={({ nativeEvent }) => + nativeEvent.event === "hide-viewed" ? filter.toggleViewedFiles() : filter.toggle() + } > @@ -182,6 +204,18 @@ function FileFilterMenu({ filter }: { readonly filter: ChangedFiles["filter"] }) function NoVisibleFiles({ files }: { readonly files: ChangedFiles }) { const { diff, filter, parsed } = files; + if (parsed.kind === "files" && parsed.files.length === 0 && filter.hiddenViewedCount > 0) { + return ( + + } + variant="plain" + /> + ); + } if (parsed.kind === "files") { return parsed.files.length === 0 && filter.hiddenCount > 0 ? ( pullRequestDiffPathMatches(linkAnchor.digest, file.path)) ?.path : undefined; + const viewed = usePullRequestFilesViewed(route.params, commit ? null : detail.data); + const reviewDraft = usePullRequestReviewDraft(route.params); + const [pickedPath, setJumpPath] = useState(null); const filter = usePullRequestFileFilter( result.parsed, - linkPath ?? (route.params.focus ? route.params.path : undefined), + viewed, + linkPath ?? (route.params.path || undefined), + [ + ...reviewDraft.draft.comments.map((comment) => comment.path), + ...(pickedPath ? [pickedPath] : []), + ], ); const { refresh: refreshDetail } = detail; useFocusEffect(refreshDetail); const { loadMore } = result; const parsed = filter.parsed; - const reviewDraft = usePullRequestReviewDraft(route.params); - const viewed = usePullRequestFilesViewed(route.params, commit ? null : detail.data); const [commitViewedPaths, setCommitViewedPaths] = useState>(() => new Set()); const permissions = usePullRequestPermissions(route.params, detail.data); const scopeKey = pullRequestReviewKey(route.params); @@ -1052,7 +1058,6 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { code: row.content ?? "", }); }; - const [pickedPath, setJumpPath] = useState(null); const jumpPath = pickedPath ?? linkPath ?? route.params.path; const requestedFile = result.parsed.kind === "files" @@ -1091,18 +1096,24 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { const [collapsedIds, setCollapsedIds] = useState(EMPTY_IDS); const [visibleFileId, setVisibleFileId] = useState(null); const nativeView = useRef(null); - // The filter the current jump was made under. Hiding or showing test files replaces the rows, - // so a jump made under the other filter has to run again. Native applies the entry + // The filters the current jump was made under. Hiding or showing files replaces the rows, + // so a jump made under other filters has to run again. Native applies the entry // destination before its first viewport, so the first one counts as made. - const jumpedUnder = useRef(targetFile ? filter.hideTestFiles : null); + const filterChoice = { + hideTestFiles: filter.hideTestFiles, + hideViewedFiles: filter.hideViewedFiles, + }; + const jumpedUnder = useRef(targetFile ? filterChoice : null); const hasTestFiles = result.parsed.kind === "files" && result.parsed.files.some((file) => isTestChangedFile(file.path)); const jumpStarted = () => - jumpedUnder.current !== null && (!hasTestFiles || jumpedUnder.current === filter.hideTestFiles); + jumpedUnder.current !== null && + (!hasTestFiles || jumpedUnder.current.hideTestFiles === filter.hideTestFiles) && + (!filter.viewedCount || jumpedUnder.current.hideViewedFiles === filter.hideViewedFiles); const scrollToFile = (fileId: string) => { if (!nativeView.current) return; - jumpedUnder.current = filter.hideTestFiles; + jumpedUnder.current = filterChoice; void nativeView.current.scrollToFile(fileId, false).catch(() => { jumpedUnder.current = null; }); @@ -1147,7 +1158,7 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { nativeView, jumpStarted, setJumpStarted: (started) => { - jumpedUnder.current = started ? filter.hideTestFiles : null; + jumpedUnder.current = started ? filterChoice : null; }, onDebug: bridge.onDebug, loadMore, @@ -1160,11 +1171,18 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { ); }; const visibleFile = data.files.find((file) => file.id === visibleFileId) ?? targetFile; - const toggleTestFiles = () => { + const keepVisibleFile = () => { setJumpPath(visibleFile?.path ?? route.params.path); setSelectedRowId(null); + }; + const toggleTestFiles = () => { + keepVisibleFile(); filter.toggle(); }; + const toggleViewedFiles = () => { + keepVisibleFile(); + filter.toggleViewedFiles(); + }; const title = commit ? "Commit changes" : "Changes"; const seekingFile = !targetFile && result.nextCursor !== null; const loadingDiff = (result.isPending && !data.rows.length) || (seekingFile && !result.error); @@ -1172,7 +1190,7 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { navigation.goBack()} menus={[ { @@ -1221,6 +1239,17 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { selected: filter.hideTestFiles, onPress: toggleTestFiles, }, + ...(filter.canHideViewedFiles + ? [ + { + id: "hide-viewed", + title: "Hide viewed files", + icon: "eye.slash", + selected: filter.hideViewedFiles, + onPress: toggleViewedFiles, + }, + ] + : []), ...(selectedRow && selectedFile && canCommentOnDiff ? [ { @@ -1363,6 +1392,25 @@ function PullRequestFileRouteContent({ route }: PullRequestFileRouteProps) { contentResetKey={`pull-request:${route.params.environmentId}:${route.params.repository}:${route.params.number}:${commit ?? "all"}`} /> + ) : filter.hiddenViewedCount > 0 && + !data.rows.length && + !result.isPending && + !result.error && + !seekingFile ? ( + + + } + variant="plain" + /> + ) : filter.hiddenCount > 0 && !data.rows.length && !result.isPending && diff --git a/apps/mobile/src/features/pull-requests/pullRequestFileFilters.test.ts b/apps/mobile/src/features/pull-requests/pullRequestFileFilters.test.ts index a7a76abc6e9e4..c5116e8451c8c 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestFileFilters.test.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestFileFilters.test.ts @@ -1,7 +1,11 @@ import { describe, expect, it } from "vite-plus/test"; import { buildNativeReviewDiffData } from "../review/nativeReviewDiffAdapter"; import { buildReviewParsedDiff } from "../review/reviewModel"; -import { filterPullRequestTestFiles } from "./pullRequestFileFilters"; +import { + filterPullRequestTestFiles, + filterPullRequestViewedFiles, + viewedFilePaths, +} from "./pullRequestFileFilters"; function patch(path: string) { return `diff --git a/${path} b/${path}\n--- a/${path}\n+++ b/${path}\n@@ -1 +1 @@\n-old\n+new\n`; @@ -42,6 +46,31 @@ describe("mobile PR test file filter", () => { expect(visible.files[1]).toBe(parsed.files[2]); }); + it("hides viewed files except those kept on screen", () => { + const parsed = buildReviewParsedDiff( + ["src/read.ts", "src/opened.ts", "src/drafted.ts", "src/unread.ts"].map(patch).join("\n"), + "pr", + ); + const paths = viewedFilePaths( + parsed, + (path) => path !== "src/unread.ts", + (path) => path === "src/opened.ts" || path === "src/drafted.ts", + ); + expect(paths).toEqual({ + viewed: ["src/read.ts", "src/opened.ts", "src/drafted.ts"], + hidden: ["src/read.ts"], + }); + const visible = filterPullRequestViewedFiles(parsed, new Set(paths.hidden)); + if (parsed.kind !== "files" || visible.kind !== "files") throw new Error("Expected files"); + expect(visible.files.map((file) => file.path)).toEqual([ + "src/opened.ts", + "src/drafted.ts", + "src/unread.ts", + ]); + expect(visible.fileCount).toBe(3); + expect(filterPullRequestViewedFiles(parsed, new Set())).toBe(parsed); + }); + it("uses the current path when a file is renamed out of a test directory", () => { const parsed = buildReviewParsedDiff( "diff --git a/tests/helper.ts b/src/helper.ts\nsimilarity index 100%\nrename from tests/helper.ts\nrename to src/helper.ts\n", diff --git a/apps/mobile/src/features/pull-requests/pullRequestFileFilters.ts b/apps/mobile/src/features/pull-requests/pullRequestFileFilters.ts index 2e0424ddbbaed..8395424692a41 100644 --- a/apps/mobile/src/features/pull-requests/pullRequestFileFilters.ts +++ b/apps/mobile/src/features/pull-requests/pullRequestFileFilters.ts @@ -1,15 +1,9 @@ import { isTestChangedFile } from "@t3tools/shared/changedFileKind"; import type { ReviewParsedDiff } from "../review/reviewModel"; -export function filterPullRequestTestFiles( - parsed: ReviewParsedDiff, - hideTestFiles: boolean, - includePath?: string, -): ReviewParsedDiff { - if (!hideTestFiles || parsed.kind !== "files") return parsed; - const files = parsed.files.filter( - (file) => file.path === includePath || !isTestChangedFile(file.path), - ); +function keepFiles(parsed: ReviewParsedDiff, keep: (path: string) => boolean): ReviewParsedDiff { + if (parsed.kind !== "files") return parsed; + const files = parsed.files.filter((file) => keep(file.path)); if (files.length === parsed.files.length) return parsed; return { ...parsed, @@ -19,3 +13,30 @@ export function filterPullRequestTestFiles( deletions: files.reduce((total, file) => total + file.deletions, 0), }; } + +export function filterPullRequestTestFiles( + parsed: ReviewParsedDiff, + hideTestFiles: boolean, + includePath?: string, +): ReviewParsedDiff { + if (!hideTestFiles) return parsed; + return keepFiles(parsed, (path) => path === includePath || !isTestChangedFile(path)); +} + +/** Viewed files among `parsed`, and those the filter hides: every one not kept on screen. */ +export function viewedFilePaths( + parsed: ReviewParsedDiff, + isViewed: (path: string) => boolean, + isKept: (path: string) => boolean, +) { + const viewed = + parsed.kind === "files" ? parsed.files.map((file) => file.path).filter(isViewed) : []; + return { viewed, hidden: viewed.filter((path) => !isKept(path)) }; +} + +export function filterPullRequestViewedFiles( + parsed: ReviewParsedDiff, + hiddenPaths: ReadonlySet, +): ReviewParsedDiff { + return keepFiles(parsed, (path) => !hiddenPaths.has(path)); +} diff --git a/apps/mobile/src/features/pull-requests/usePullRequestFileFilter.ts b/apps/mobile/src/features/pull-requests/usePullRequestFileFilter.ts index d8d824c1ae5fc..5651df8679644 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestFileFilter.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestFileFilter.ts @@ -3,24 +3,61 @@ import { AsyncResult } from "effect/reactivity"; import { useMemo } from "react"; import type { ReviewParsedDiff } from "../review/reviewModel"; import { mobilePreferencesAtom, updateMobilePreferencesAtom } from "../../state/preferences"; -import { filterPullRequestTestFiles } from "./pullRequestFileFilters"; +import { + filterPullRequestTestFiles, + filterPullRequestViewedFiles, + viewedFilePaths, +} from "./pullRequestFileFilters"; +import type { usePullRequestFilesViewed } from "./usePullRequestFilesViewed"; -export function usePullRequestFileFilter(parsed: ReviewParsedDiff, includePath?: string) { +const PATH_SEPARATOR = "\0"; + +export function usePullRequestFileFilter( + parsed: ReviewParsedDiff, + viewed: ReturnType, + includePath?: string, + keptPaths: readonly string[] = [], +) { const preferences = useAtomValue(mobilePreferencesAtom); const savePreferences = useAtomSet(updateMobilePreferencesAtom); - const hideTestFiles = - AsyncResult.isSuccess(preferences) && preferences.value.pullRequestHideTestFiles === true; - const visible = useMemo( + const saved = AsyncResult.isSuccess(preferences) ? preferences.value : null; + const hideTestFiles = saved?.pullRequestHideTestFiles === true; + const hideViewedFiles = viewed.supported && saved?.pullRequestHideViewedFiles === true; + const withoutTests = useMemo( () => filterPullRequestTestFiles(parsed, hideTestFiles, includePath), [parsed, hideTestFiles, includePath], ); + const { viewed: viewedPaths, hidden } = viewedFilePaths( + withoutTests, + (path) => viewed.supported && viewed.isViewed(path), + // Files marked here stay until the screen closes, so marking one never pulls it out from under you. + (path) => path === includePath || keptPaths.includes(path) || viewed.toggledHere(path), + ); + const hiddenViewedPaths = hideViewedFiles ? hidden : []; + // A string key keeps the same diff object while the hidden set holds, so native rows aren't rebuilt. + const hiddenViewedKey = hiddenViewedPaths.join(PATH_SEPARATOR); + const visible = useMemo( + () => + hiddenViewedKey + ? filterPullRequestViewedFiles(withoutTests, new Set(hiddenViewedKey.split(PATH_SEPARATOR))) + : withoutTests, + [withoutTests, hiddenViewedKey], + ); const hiddenCount = - parsed.kind === "files" && visible.kind === "files" ? parsed.fileCount - visible.fileCount : 0; + parsed.kind === "files" && withoutTests.kind === "files" + ? parsed.fileCount - withoutTests.fileCount + : 0; return { parsed: visible, hideTestFiles, hiddenCount, toggle: () => savePreferences({ pullRequestHideTestFiles: !hideTestFiles }), showAll: () => savePreferences({ pullRequestHideTestFiles: false }), + hideViewedFiles, + viewedCount: viewedPaths.length, + hiddenViewedCount: hiddenViewedPaths.length, + canHideViewedFiles: viewed.supported && (viewedPaths.length > 0 || hideViewedFiles), + toggleViewedFiles: () => savePreferences({ pullRequestHideViewedFiles: !hideViewedFiles }), + showViewedFiles: () => savePreferences({ pullRequestHideViewedFiles: false }), }; } diff --git a/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.ts b/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.ts index c973c81dac39e..d3f71a3a6a59e 100644 --- a/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.ts +++ b/apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.ts @@ -30,6 +30,9 @@ export function usePullRequestFilesViewed(params: PullRequestParams, pr: PullReq const reads = useAtomValue(readsAtom); const write = useAtomCommand(composerPullRequests.setFilesViewed, { reportFailure: false }); const [state, setState] = useState({ key: scopeKey, presses: {}, error: null }); + // Kept apart from `presses`, which a failed write clears, so a file marked here stays listed. + const [marked, setMarked] = useState({ key: scopeKey, paths: new Set() }); + const markedPaths = marked.key === scopeKey ? marked.paths : null; const presses = state.key === scopeKey ? state.presses : {}; const error = state.key === scopeKey ? state.error : null; const overlay = viewedOverlay(presses, reads); @@ -44,6 +47,10 @@ export function usePullRequestFilesViewed(params: PullRequestParams, pr: PullReq if (!supported || !allowed || !query.data) return; const viewed = !isViewed(path); const version = (presses[path]?.version ?? 0) + 1; + setMarked((current) => ({ + key: scopeKey, + paths: new Set(current.key === scopeKey ? current.paths : []).add(path), + })); update((current) => ({ ...current, error: null, @@ -80,6 +87,7 @@ export function usePullRequestFilesViewed(params: PullRequestParams, pr: PullReq supported, canToggle: supported && allowed && Boolean(query.data), isViewed, + toggledHere: (path: string) => markedPaths?.has(path) === true, toggle, error: error ?? query.error, truncated: query.data?.truncated, diff --git a/apps/mobile/src/persistence/mobile-preferences.ts b/apps/mobile/src/persistence/mobile-preferences.ts index 7a26fc2b8d694..0124189767d9f 100644 --- a/apps/mobile/src/persistence/mobile-preferences.ts +++ b/apps/mobile/src/persistence/mobile-preferences.ts @@ -99,6 +99,7 @@ export interface Preferences { readonly navigationTabOrder?: readonly string[]; readonly hiddenNavigationTabs?: readonly string[]; readonly pullRequestHideTestFiles?: boolean; + readonly pullRequestHideViewedFiles?: boolean; /** Which hidden-by-default PR activity is shown: bot comments and resolved or dismissed ones. */ readonly pullRequestActivityVisibility?: PullRequestActivityVisibility; readonly pullRequestMergeMethod?: PullRequestMergeMethod; @@ -190,6 +191,7 @@ function sanitizePreferences(parsed: Preferences): Preferences { navigationTabOrder?: readonly string[]; hiddenNavigationTabs?: readonly string[]; pullRequestHideTestFiles?: boolean; + pullRequestHideViewedFiles?: boolean; pullRequestActivityVisibility?: PullRequestActivityVisibility; pullRequestMergeMethod?: PullRequestMergeMethod; liveActivitiesEnabled?: boolean; @@ -243,6 +245,10 @@ function sanitizePreferences(parsed: Preferences): Preferences { preferences.pullRequestHideTestFiles = parsed.pullRequestHideTestFiles; } + if (typeof parsed.pullRequestHideViewedFiles === "boolean") { + preferences.pullRequestHideViewedFiles = parsed.pullRequestHideViewedFiles; + } + if (isPullRequestActivityVisibility(parsed.pullRequestActivityVisibility)) { preferences.pullRequestActivityVisibility = parsed.pullRequestActivityVisibility; }