Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion apps/mobile/src/features/review/ReviewSheet.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,10 @@ export function ReviewSheet(props: ReviewSheetProps) {
cwd: selectedThreadCwd,
selectedSection,
draftMessage,
expandedFileIds: selectedSection?.id
? reviewCache.expandedFileIdsBySection[selectedSection.id]
: undefined,
enabled: isEnvironmentReady,
});
// Expo may return null before registration or in older binaries; use the fallback.
const NativeReviewDiffView = resolveNativeReviewDiffView();
Expand Down Expand Up @@ -567,7 +571,7 @@ export function ReviewSheet(props: ReviewSheetProps) {
const handleNativeToggleFile = (event: NativeSyntheticEvent<{ readonly fileId?: string }>) => {
const { fileId } = event.nativeEvent;
if (fileId) {
loadVisibleFile(fileId, true);
if (collapsedFileIds.includes(fileId)) loadVisibleFile(fileId, true);
toggleExpandedFile(fileId);
}
};
Expand Down
41 changes: 41 additions & 0 deletions apps/mobile/src/features/review/reviewFilePatchDemand.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { describe, expect, it } from "vite-plus/test";

import { getReviewFilePatchLookahead } from "./reviewFilePatchDemand";

const files = Array.from({ length: 30 }, (_, index) => ({
path: `file-${index}.ts`,
previousPath: null,
additions: 1,
deletions: 1,
}));

describe("getReviewFilePatchLookahead", () => {
it("preserves default expanded viewport lookahead", () => {
expect(getReviewFilePatchLookahead(files, 0, undefined)).toEqual([
"file-0.ts",
"file-1.ts",
"file-2.ts",
]);
});

it("does not request collapsed files or scan past the lookahead window", () => {
expect(getReviewFilePatchLookahead(files, 10, ["file-11.ts", "file-20.ts"])).toEqual([
"file-11.ts",
]);
expect(getReviewFilePatchLookahead(files, 0, [])).toEqual([]);
});

it("keeps a requested path valid when another file is inserted before it", () => {
const requested = getReviewFilePatchLookahead(files, 10, ["file-10.ts"]);
const updated = [{ ...files[0]!, path: "earlier.ts" }, ...files];

expect(
updated.filter((file) => requested.includes(file.path)).map((file) => file.path),
).toEqual(["file-10.ts"]);
});

it("bounds the window at the last file and ignores missing selections", () => {
expect(getReviewFilePatchLookahead(files, 29, undefined)).toEqual(["file-29.ts"]);
expect(getReviewFilePatchLookahead(files, -1, undefined)).toEqual([]);
});
});
14 changes: 14 additions & 0 deletions apps/mobile/src/features/review/reviewFilePatchDemand.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import type { ReviewDiffFileStat } from "@t3tools/contracts";

export function getReviewFilePatchLookahead(
files: ReadonlyArray<ReviewDiffFileStat>,
startIndex: number,
expandedFileIds: ReadonlyArray<string> | undefined,
): ReadonlyArray<string> {
if (startIndex < 0) return [];

return files
.slice(startIndex, startIndex + 3)
.filter((file) => expandedFileIds === undefined || expandedFileIds.includes(file.path))
.map((file) => file.path);
}
42 changes: 30 additions & 12 deletions apps/mobile/src/features/review/useReviewDiffData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { formatDiffCount } from "@t3tools/client-runtime/diff-count";
import { countReviewCommentContexts, parseReviewInlineComments } from "./reviewCommentSelection";
import { getCachedNativeReviewDiffData } from "./nativeReviewDiffAdapter";
import { getCachedReviewParsedDiff } from "./reviewState";
import { getReviewFilePatchLookahead } from "./reviewFilePatchDemand";
import {
applyReviewDiffMetadata,
buildReviewParsedDiff,
Expand Down Expand Up @@ -126,6 +127,8 @@ export function useReviewDiffData(input: {
readonly cwd: string | null;
readonly selectedSection: ReviewSectionItem | null;
readonly draftMessage: string;
readonly expandedFileIds: ReadonlyArray<string> | undefined;
readonly enabled: boolean;
}) {
const { draftMessage, selectedSection, threadKey } = input;
const selectedSectionId = selectedSection?.id ?? null;
Expand All @@ -147,17 +150,24 @@ export function useReviewDiffData(input: {
source?.baseRef,
source?.diffHash,
]);
const [requested, setRequested] = useState({ scope, indices: [0, 1, 2] });
const indices = useMemo(
() => new Set(requested.scope === scope ? requested.indices : [0, 1, 2]),
[requested, scope],
const initialPaths = useMemo(
() => getReviewFilePatchLookahead(lazySource?.files ?? [], 0, input.expandedFileIds),
[input.expandedFileIds, lazySource?.files],
);
const [requested, setRequested] = useState<{ scope: string; paths: ReadonlyArray<string> }>({
scope,
paths: [],
});
const requestedPaths = useMemo(
() => new Set([...(requested.scope === scope ? requested.paths : []), ...initialPaths]),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
[initialPaths, requested, scope],
);
const queries = useMemo(
() =>
!environmentId || !cwd || !lazySource
!input.enabled || !environmentId || !cwd || !lazySource
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
? []
: (lazySource.files ?? []).map((file, index) =>
indices.has(index)
: (lazySource.files ?? []).map((file) =>
requestedPaths.has(file.path)
? reviewEnvironment.diffFilePatch({
environmentId,
input: {
Expand All @@ -177,7 +187,7 @@ export function useReviewDiffData(input: {
})
: null,
),
[environmentId, cwd, lazySource, indices, scope],
[input.enabled, environmentId, cwd, lazySource, requestedPaths, scope],
);
const parsedQuery = useMemo(
() =>
Expand All @@ -203,14 +213,22 @@ export function useReviewDiffData(input: {
fileId === null ? 0 : (lazySource?.files?.findIndex((file) => file.path === fileId) ?? -1);
if (index < 0) return;
setRequested((current) => {
const previous = current.scope === scope ? current.indices : [0, 1, 2];
const added = [index, index + 1, index + 2].filter((next) => !previous.includes(next));
return added.length === 0 ? current : { scope, indices: [...previous, ...added] };
const previous = current.scope === scope ? current.paths : initialPaths;
const expandedFileIds =
retry && fileId !== null && input.expandedFileIds !== undefined
? [...input.expandedFileIds, fileId]
: input.expandedFileIds;
const added = getReviewFilePatchLookahead(
lazySource?.files ?? [],
index,
expandedFileIds,
).filter((path) => !previous.includes(path));
return added.length === 0 ? current : { scope, paths: [...previous, ...added] };
});
if (retry && patches[index]?._tag === "Failure" && queries[index])
registry.refresh(queries[index]);
},
[lazySource, scope, patches, queries, registry],
[initialPaths, input.expandedFileIds, lazySource, scope, patches, queries, registry],
);
const parsedDiff = useMemo<ReviewParsedDiff>(() => {
if (!lazySource?.files) return applyReviewDiffMetadata(previewDiff, selectedSection);
Expand Down
69 changes: 69 additions & 0 deletions apps/mobile/src/features/review/useReviewDiffPrewarming.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { describe, expect, it, vi } from "vite-plus/test";

import { getCachedNativeReviewDiffData } from "./nativeReviewDiffAdapter";
import type { ReviewSectionItem } from "./reviewModel";
import { getCachedReviewParsedDiff } from "./reviewState";
import { getReviewDiffPrewarmSections, prewarmReviewDiffSection } from "./useReviewDiffPrewarming";

vi.mock("./nativeReviewDiffAdapter", () => ({ getCachedNativeReviewDiffData: vi.fn() }));
vi.mock("./reviewState", () => ({
getCachedReviewParsedDiff: vi.fn(() => ({ kind: "empty" })),
getReviewParsedDiffSourceCharacterCount: (input: { diff: string | null }) =>
input.diff?.length ?? 0,
MAX_CACHED_REVIEW_DIFFS: 8,
MAX_CACHED_REVIEW_SOURCE_CHARACTERS: 4_194_304,
}));

function section(id: string, lazy = false): ReviewSectionItem {
return {
id,
kind: "working-tree",
title: id,
subtitle: null,
diff: "patch",
isLoading: false,
source: {
id,
kind: "working-tree",
title: id,
diff: "patch",
diffHash: id,
truncated: lazy,
files: [],
baseRef: null,
headRef: null,
},
};
}

describe("review diff prewarming", () => {
it("skips truncated stat-backed neighbors whose bodies use file queries", () => {
const sections = [section("selected"), section("lazy", true), section("complete")];

expect(
getReviewDiffPrewarmSections({
threadKey: "thread",
sections,
selectedSectionId: "selected",
}).map((item) => item.id),
).toEqual(["complete"]);
});

it("does not parse or prepare a lazy source even when invoked directly", () => {
vi.clearAllMocks();

prewarmReviewDiffSection({ threadKey: "thread", section: section("lazy", true) });

expect(getCachedReviewParsedDiff).not.toHaveBeenCalled();
expect(getCachedNativeReviewDiffData).not.toHaveBeenCalled();
});

it("still prewarms complete section bodies", () => {
vi.clearAllMocks();

prewarmReviewDiffSection({ threadKey: "thread", section: section("complete") });

expect(getCachedReviewParsedDiff).toHaveBeenCalledOnce();
expect(getCachedNativeReviewDiffData).toHaveBeenCalledOnce();
});
});
16 changes: 12 additions & 4 deletions apps/mobile/src/features/review/useReviewDiffPrewarming.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,16 @@ function cancelIdle(handle: number): void {
clearTimeout(handle);
}

function prewarmReviewDiffSection(input: {
export function prewarmReviewDiffSection(input: {
readonly threadKey: string;
readonly section: ReviewSectionItem;
}): void {
const { section, threadKey } = input;
if (section.diff === null || section.diff.length > MAX_CACHED_REVIEW_SOURCE_CHARACTERS) {
if (
section.diff === null ||
section.diff.length > MAX_CACHED_REVIEW_SOURCE_CHARACTERS ||
(section.source?.truncated && section.source.files)
) {
return;
}

Expand All @@ -52,7 +56,7 @@ function prewarmReviewDiffSection(input: {
getCachedNativeReviewDiffData({ parsedDiff, comments: [] });
}

function getReviewDiffPrewarmSections(input: {
export function getReviewDiffPrewarmSections(input: {
readonly threadKey: string;
readonly sections: ReadonlyArray<ReviewSectionItem>;
readonly selectedSectionId: string | null;
Expand Down Expand Up @@ -80,7 +84,11 @@ function getReviewDiffPrewarmSections(input: {
return pendingSections;
}
const section = sections[index];
if (!section || section.diff === null) {
if (
!section ||
section.diff === null ||
(section.source?.truncated && section.source.files)
) {
continue;
}
const sectionCharacterCount = getReviewParsedDiffSourceCharacterCount({
Expand Down
2 changes: 1 addition & 1 deletion apps/mobile/src/features/review/useReviewSections.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export function useReviewSections(input: {

const selectedCheckpoint =
selectedSectionId === null ? undefined : checkpointBySectionId.get(selectedSectionId);
const activeCheckpoint = selectedCheckpoint ?? readyCheckpoints[0] ?? null;
const activeCheckpoint = selectedCheckpoint ?? null;
const activeSectionId = activeCheckpoint
? getReviewSectionIdForCheckpoint(activeCheckpoint)
: null;
Expand Down
Loading
Loading