Skip to content

Commit eb353fd

Browse files
jakeleventhaljakeleventhal
committed
fix(review): unify file categories and stabilize chapter counts
1 parent 1913168 commit eb353fd

13 files changed

Lines changed: 204 additions & 109 deletions

‎apps/mobile/src/features/pull-requests/PullRequestRouteScreen.tsx‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -977,7 +977,7 @@ export function PullRequestDiffContent({
977977
() =>
978978
pullRequestFileChapters({
979979
completePaths:
980-
!detail.data || (viewed.supported && viewed.filePaths === null)
980+
!detail.data || (viewed.supported && (!viewed.ready || viewed.truncated))
981981
? null
982982
: !commit && viewed.filePaths !== null
983983
? viewed.filePaths
@@ -998,6 +998,8 @@ export function PullRequestDiffContent({
998998
commit,
999999
detail.data,
10001000
viewed.supported,
1001+
viewed.ready,
1002+
viewed.truncated,
10011003
viewed.filePaths,
10021004
result.isPending,
10031005
result.error,

‎apps/mobile/src/features/pull-requests/pullRequestFileChapters.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ describe("mobile PR chapter counts", () => {
1616
});
1717
expect(result.complete).toBe(true);
1818
expect(result.chapters.map((chapter) => chapter.title)).toEqual([
19-
"Implementation",
19+
"Code",
2020
"Tests",
2121
"Generated",
2222
]);
@@ -52,7 +52,7 @@ describe("mobile PR chapter counts", () => {
5252
chapter.visibleFiles.length,
5353
]),
5454
).toEqual([
55-
["implementation", 79, 0],
55+
["code", 79, 0],
5656
["tests", 1, 0],
5757
]);
5858
});

‎apps/mobile/src/features/pull-requests/pullRequestFileChapters.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { reviewFileChapters, isReviewTestFile } from "@t3tools/shared/reviewFileClassification";
1+
import { reviewFileChapterIndex, isReviewTestFile } from "@t3tools/shared/reviewFileClassification";
22

33
export function pullRequestFileChapters(input: {
44
readonly completePaths: ReadonlyArray<string> | null;
@@ -11,8 +11,10 @@ export function pullRequestFileChapters(input: {
1111
const paths = (input.completePaths ?? input.loadedPaths).filter(
1212
(path) => !input.hideTestFiles || path === input.includePath || !isReviewTestFile(path),
1313
);
14-
return {
15-
complete: input.completePaths !== null,
16-
chapters: reviewFileChapters(paths, (path) => path, input.visiblePaths),
17-
};
14+
return reviewFileChapterIndex({
15+
completePaths: input.completePaths === null ? null : paths,
16+
loadedFiles: paths,
17+
visibleFiles: input.visiblePaths,
18+
getPath: (path) => path,
19+
});
1820
}

‎apps/mobile/src/features/pull-requests/pullRequestFileFilters.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,13 @@ function patch(path: string) {
88
}
99

1010
describe("mobile PR test file filter", () => {
11-
it("recognizes test names, test directories and snapshots across common languages", () => {
11+
it("recognizes test names and directories while keeping mocks in their own category", () => {
1212
const paths = [
1313
"src/app.test.ts",
1414
"src/app.spec.tsx",
1515
"src/__tests__/app.ts",
16-
"tests/helpers/fixtures.json",
1716
"test/app.js",
1817
"e2e/login.ts",
19-
"src/__snapshots__/app.snap",
20-
"src/app.test.ts.snap",
2118
"test_app.py",
2219
"app_test.py",
2320
"internal/app_test.go",
@@ -40,6 +37,9 @@ describe("mobile PR test file filter", () => {
4037
"specification.md",
4138
"AppTest.ts",
4239
"pnpm-lock.yaml",
40+
"src/app.test.ts.snap",
41+
"src/__snapshots__/app.snap",
42+
"tests/helpers/fixtures.json",
4343
].filter(isPullRequestTestFile),
4444
).toEqual([]);
4545
});

‎apps/mobile/src/features/pull-requests/usePullRequestFilesViewed.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,13 @@ export function usePullRequestFilesViewed(params: PullRequestParams, pr: PullReq
3737
save,
3838
onError,
3939
});
40+
// Environment-backed marks are sparse; only a host-backed read inventories unviewed files too.
4041
const filePaths = useMemo(
41-
() => (query.data && !query.data.truncated ? query.data.files.map((file) => file.path) : null),
42-
[query.data],
42+
() =>
43+
pr?.capabilities.viewedFiles === "host" && query.data && !query.data.truncated
44+
? query.data.files.map((file) => file.path)
45+
: null,
46+
[pr?.capabilities.viewedFiles, query.data],
4347
);
4448
const canToggle = supported && connected && permission && Boolean(query.data);
4549
const setViewed = (path: string, viewed: boolean) => {
@@ -53,6 +57,7 @@ export function usePullRequestFilesViewed(params: PullRequestParams, pr: PullReq
5357
return {
5458
...progress,
5559
supported,
60+
ready: query.data !== null,
5661
filePaths,
5762
canToggle,
5863
error: error ?? query.error,

‎apps/web/src/components/pullRequest/PullRequestCodeTab.tsx‎

Lines changed: 70 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import {
2-
reviewFileChapters,
2+
reviewFileChapterIndex,
33
reviewFileGroup,
44
type ReviewFileGroup,
55
} from "@t3tools/shared/reviewFileClassification";
@@ -366,7 +366,7 @@ function PullRequestCodeTab({
366366
const commentsOnly = !guideFallback && settings.pullRequestViewing.commentsOnly;
367367
const hideReviewed = !guideFallback && settings.pullRequestViewing.hideReviewed;
368368
const groupByType = guideFallback || settings.pullRequestViewing.groupByType;
369-
const [selectedChapter, setSelectedChapter] = useState<ReviewFileGroup>("implementation");
369+
const [selectedChapter, setSelectedChapter] = useState<ReviewFileGroup>("code");
370370
const [chapterRailOpen, setChapterRailOpen] = useState(false);
371371
const canSetChapterViewed = useAtomValue(
372372
pullRequestEnvironment.setFilesViewed.permissionAtom(environmentId),
@@ -416,7 +416,7 @@ function PullRequestCodeTab({
416416
setFoldOverride(null);
417417
setVisibleCommitCount(COMMIT_PAGE_SIZE);
418418
setOrphansOpen(false);
419-
setSelectedChapter("implementation");
419+
setSelectedChapter("code");
420420
setChapterRailOpen(false);
421421
setSliceState({ key: scopeKey, cursor: null, slices: NO_SLICES });
422422
}
@@ -741,15 +741,34 @@ function PullRequestCodeTab({
741741
(commentCounts.get(path)?.unresolved ?? 0) > 0,
742742
),
743743
);
744-
const chapters = groupByType
745-
? reviewFileChapters(annotatedFiles, (file) => file.path, filteredFiles)
746-
: [];
744+
const diffComplete =
745+
loadedSlices.length > 0 &&
746+
nextCursor === null &&
747+
!diffQuery.isPending &&
748+
diffQuery.error === null;
749+
const chapterIndex = reviewFileChapterIndex({
750+
completePaths:
751+
commit === null && viewedFilesStore === "host" && filesViewed.filePaths !== null
752+
? filesViewed.filePaths
753+
: diffComplete && !withheldContent
754+
? filePaths
755+
: null,
756+
loadedFiles: annotatedFiles,
757+
visibleFiles: filteredFiles,
758+
getPath: (file) => file.path,
759+
});
760+
const chapters = groupByType ? chapterIndex.chapters : [];
747761
const chapterPosition = Math.max(
748762
0,
749763
chapters.findIndex((chapter) => chapter.id === selectedChapter),
750764
);
751765
const chapter = chapters[chapterPosition] ?? null;
752-
const chapterReviewComplete = nextCursor === null && !diffQuery.isPending;
766+
const chapterReviewComplete =
767+
chapterIndex.complete && (!filesViewedEnabled || (filesViewed.ready && !filesViewed.truncated));
768+
const chapterCountsPending =
769+
filesViewed.error || filesViewed.truncated || diffQuery.error
770+
? "File counts unavailable"
771+
: "Loading file counts…";
753772
const items = ((): CodeViewDiffItem<ReviewAnnotationGroup>[] =>
754773
(chapter?.visibleFiles ?? filteredFiles).map(
755774
({ fileKey, path, fileDiff, annotations, annotationsVersion }) => {
@@ -884,28 +903,41 @@ function PullRequestCodeTab({
884903
);
885904
};
886905

906+
const setChapterViewed = (paths: ReadonlyArray<string>, reviewed: boolean) => {
907+
const selected = new Set(paths);
908+
const remaining = new Set(paths);
909+
for (const file of annotatedFiles) {
910+
if (!selected.has(file.path)) continue;
911+
setFileViewed(file.fileKey, file.path, reviewed);
912+
remaining.delete(file.path);
913+
}
914+
for (const path of remaining) setViewed(path, reviewed);
915+
};
887916
const chapterSummaries = chapters.map((candidate, index) => {
888917
const members = candidate.files;
889-
const reviewedCount = members.filter((file) => isFileViewed(file.path)).length;
890-
const stat = members.reduce(
891-
(total, file) => {
892-
const next = sourceFileStats.get(file.fileKey) ?? getDiffLineStat([file.fileDiff]);
893-
return {
894-
additions: total.additions + next.additions,
895-
deletions: total.deletions + next.deletions,
896-
};
897-
},
898-
{ additions: 0, deletions: 0 },
899-
);
918+
const reviewedCount = members.filter(isFileViewed).length;
919+
const stat = annotatedFiles
920+
.filter((file) => reviewFileGroup(file.path) === candidate.id)
921+
.reduce(
922+
(total, file) => {
923+
const next = sourceFileStats.get(file.fileKey) ?? getDiffLineStat([file.fileDiff]);
924+
return {
925+
additions: total.additions + next.additions,
926+
deletions: total.deletions + next.deletions,
927+
};
928+
},
929+
{ additions: 0, deletions: 0 },
930+
);
900931
return {
901932
title: candidate.title,
902933
reviewed: chapterReviewComplete && reviewedCount === members.length,
903-
stat,
934+
stat: diffComplete ? stat : null,
904935
detail:
905936
index !== chapterPosition ? (
906937
<span>
907-
{reviewedCount} of {members.length} files reviewed
908-
{chapterReviewComplete ? "" : " (loaded)"}
938+
{chapterReviewComplete
939+
? `${reviewedCount} of ${members.length} files reviewed`
940+
: chapterCountsPending}
909941
</span>
910942
) : null,
911943
...(filesViewed.enabled && canSetChapterViewed
@@ -915,7 +947,7 @@ function PullRequestCodeTab({
915947
fileCount: members.length,
916948
disabled: !chapterReviewComplete,
917949
onSetReviewed: (reviewed: boolean) => {
918-
for (const file of members) setFileViewed(file.fileKey, file.path, reviewed);
950+
setChapterViewed(members, reviewed);
919951
},
920952
},
921953
}
@@ -1734,29 +1766,28 @@ function PullRequestCodeTab({
17341766
ariaLabel="Code review chapters"
17351767
chapters={chapterSummaries.map((summary, index) => {
17361768
const members = chapters[index]?.files ?? [];
1737-
const reviewedCount = members.filter((file) => isFileViewed(file.path)).length;
1769+
const reviewedCount = members.filter(isFileViewed).length;
17381770
return {
17391771
...summary,
17401772
selectedContent:
17411773
index === chapterPosition ? (
17421774
<div className="flex min-w-0 flex-col gap-2 px-2 pb-2">
1743-
<ReviewChapterProgress
1744-
reviewedCount={reviewedCount}
1745-
fileCount={members.length}
1746-
disabled={!chapterReviewComplete}
1747-
onSetReviewed={
1748-
filesViewed.enabled && canSetChapterViewed
1749-
? (reviewed) => {
1750-
for (const file of members)
1751-
setFileViewed(file.fileKey, file.path, reviewed);
1752-
}
1753-
: null
1754-
}
1755-
/>
1775+
{chapterReviewComplete ? (
1776+
<ReviewChapterProgress
1777+
reviewedCount={reviewedCount}
1778+
fileCount={members.length}
1779+
disabled={!chapterReviewComplete}
1780+
onSetReviewed={
1781+
filesViewed.enabled && canSetChapterViewed
1782+
? (reviewed) => {
1783+
setChapterViewed(members, reviewed);
1784+
}
1785+
: null
1786+
}
1787+
/>
1788+
) : null}
17561789
{!chapterReviewComplete ? (
1757-
<p className="text-xs text-muted-foreground">
1758-
Load all files to review this chapter.
1759-
</p>
1790+
<p className="text-xs text-muted-foreground">{chapterCountsPending}</p>
17601791
) : null}
17611792
{guideFallback ? (
17621793
<ul className="flex min-w-0 flex-col gap-0.5">

‎apps/web/src/components/pullRequest/pullRequestFileOrder.logic.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,12 @@ describe("review reading order", () => {
3838
]),
3939
).toHaveLength(2);
4040
});
41-
it("groups implementation, tests and generated files only when requested", () => {
42-
const files = ["src/z.test.ts", "src/_generated/a.ts", "src/a.ts"].map(file);
41+
it("groups code, tests and generated files only when requested", () => {
42+
const files = ["src/z.test.ts", "src/__generated__/a.ts", "src/a.ts"].map(file);
4343
expect(orderDiffFiles(files, true).map((file) => file.name)).toEqual([
4444
"src/a.ts",
4545
"src/z.test.ts",
46-
"src/_generated/a.ts",
46+
"src/__generated__/a.ts",
4747
]);
4848
});
4949
});

‎apps/web/src/components/pullRequest/usePullRequestFilesViewed.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ export function usePullRequestFilesViewed({
2828
const onError = () => {
2929
toastManager.add({ type: "error", title: "Could not update viewed files" });
3030
};
31-
return useReviewFileProgress({
31+
const progress = useReviewFileProgress({
3232
scopeKey: `${environmentId} ${reference.projectId} ${reference.host ?? ""} ${reference.repository} ${reference.number}`,
3333
enabled,
3434
paths,
@@ -37,4 +37,7 @@ export function usePullRequestFilesViewed({
3737
save,
3838
onError,
3939
});
40+
const filePaths =
41+
query.data && !query.data.truncated ? query.data.files.map((file) => file.path) : null;
42+
return { ...progress, filePaths, ready: query.data !== null };
4043
}

‎packages/client-runtime/src/reviewWorkspace.test.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,18 +106,22 @@ describe("shared review workspace", () => {
106106
expect(reviewFolderContains("src/a", "src/another.ts")).toBe(false);
107107
expect(reviewFolderContains("src/a", "src/a/z.ts")).toBe(true);
108108
});
109-
it("groups tests and generated output without cloning file metadata", () => {
109+
it("groups every diff category without cloning file metadata", () => {
110110
const files = [
111111
{ path: "pnpm-lock.yaml", id: 1 },
112112
{ path: "src/foo.test.ts", id: 2 },
113113
{ path: "src/foo.ts", id: 3 },
114+
{ path: "src/foo.fixtures.ts", id: 4 },
115+
{ path: "README.md", id: 5 },
114116
];
115117
expect(orderReviewFiles(files, (file) => file.path, true)).toEqual([
116118
files[2],
117119
files[1],
120+
files[3],
118121
files[0],
122+
files[4],
119123
]);
120-
expect(reviewFileGroup("src/__snapshots__/foo.snap")).toBe("generated");
124+
expect(reviewFileGroup("src/__snapshots__/foo.snap")).toBe("mocks");
121125
expect(reviewFileGroup("Tests/FooTests.swift")).toBe("tests");
122126
});
123127
it("attributes unresolved conversations on renamed files to their current path", () => {

‎packages/client-runtime/src/reviewWorkspace.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { reviewFileGroup } from "@t3tools/shared/reviewFileClassification";
1+
import { REVIEW_FILE_GROUPS, reviewFileGroup } from "@t3tools/shared/reviewFileClassification";
22
import type {
33
PullRequestCheck,
44
PullRequestChecksState,
@@ -103,10 +103,13 @@ export function orderReviewFiles<T>(
103103
pathOf: (file: T) => string,
104104
grouped = false,
105105
): T[] {
106-
const rank = { implementation: 0, tests: 1, generated: 2 };
106+
const rank = (path: string) => {
107+
const kind = reviewFileGroup(path);
108+
return REVIEW_FILE_GROUPS.findIndex((group) => group.id === kind);
109+
};
107110
return [...files].sort(
108111
(a, b) =>
109-
(grouped ? rank[reviewFileGroup(pathOf(a))] - rank[reviewFileGroup(pathOf(b))] : 0) ||
112+
(grouped ? rank(pathOf(a)) - rank(pathOf(b)) : 0) ||
110113
compareReviewFilePaths(pathOf(a), pathOf(b)),
111114
);
112115
}

0 commit comments

Comments
 (0)