Skip to content

Commit b4dcf64

Browse files
committed
fix(web): expand pending diff files on the first click
1 parent e9b07f6 commit b4dcf64

2 files changed

Lines changed: 64 additions & 10 deletions

File tree

‎apps/web/src/components/DiffPanel.browser.test.tsx‎

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
type ClientSettings,
99
type ReviewBranchCommitsResult,
1010
type ReviewDiffPreviewResult,
11+
type ReviewDiffPreviewSource,
1112
type ScopedThreadRef,
1213
type VcsListRefsResult,
1314
type VcsStatusResult,
@@ -18,7 +19,7 @@ import { expect, it, vi } from "vite-plus/test";
1819
import { page, userEvent } from "vite-plus/test/browser";
1920

2021
import "~/index.css";
21-
import { render } from "../../test/render";
22+
import { act, render } from "../../test/render";
2223
import { makeThreadFixture } from "~/test-fixtures";
2324

2425
type ResultAtom<A> = Atom.Atom<AsyncResult.AsyncResult<A, Error>>;
@@ -27,6 +28,8 @@ const state = vi.hoisted(() => ({
2728
thread: null as ReturnType<typeof makeThreadFixture> | null,
2829
threadRef: null as ScopedThreadRef | null,
2930
previews: new Map<string | null, ResultAtom<ReviewDiffPreviewResult>>(),
31+
patch: null as unknown as ResultAtom<ReviewDiffPreviewSource>,
32+
diffFilesCollapsed: false,
3033
config: null as unknown as Atom.Atom<null>,
3134
status: null as unknown as ResultAtom<VcsStatusResult>,
3235
localRefs: null as unknown as ResultAtom<VcsListRefsResult>,
@@ -42,6 +45,7 @@ vi.mock("~/state/review", () => ({
4245
diffPreview: ({ input }: { input: { baseRef?: string } }) =>
4346
state.previews.get(input.baseRef ?? null)!,
4447
branchCommits: () => state.commits,
48+
diffFilePatch: () => state.patch,
4549
diffFileContents: {},
4650
},
4751
}));
@@ -64,10 +68,14 @@ vi.mock("~/state/filesystem", () => ({
6468
vi.mock("~/hooks/useThreadRouteTarget", () => ({ useThreadRouteRef: () => state.threadRef }));
6569
vi.mock("~/hooks/useSettings", () => ({
6670
useClientSettings: (select: (settings: ClientSettings) => unknown) =>
67-
select(DEFAULT_CLIENT_SETTINGS),
71+
select({ ...DEFAULT_CLIENT_SETTINGS, diffFilesCollapsed: state.diffFilesCollapsed }),
6872
updateClientSettings: async () => {},
6973
}));
70-
vi.mock("~/hooks/useTheme", () => ({ useRenderedAppearance: () => "dark" }));
74+
vi.mock("~/hooks/useTheme", () => ({
75+
useRenderedAppearance: () => "dark",
76+
getRenderedAppearance: () => "dark",
77+
subscribeRenderedAppearance: () => () => {},
78+
}));
7179
vi.mock("~/state/queries", () => ({
7280
useCheckpointDiff: () => ({ data: null, error: null, isPending: false }),
7381
useTurnItemDetail: () => ({ data: null, error: null, isPending: false }),
@@ -76,8 +84,6 @@ vi.mock("~/state/use-atom-command", () => ({ useAtomCommand: () => async () => {
7684
vi.mock("~/editorPreferences", () => ({ useOpenInPreferredEditor: () => async () => {} }));
7785
vi.mock("~/fileContextMenu", () => ({ useFileContextMenuHandler: () => () => {} }));
7886
vi.mock("~/browser/useOpenLink", () => ({ useOpenLink: () => async () => {} }));
79-
// The test drives comparison controls, without mounting virtualized diff viewers or their workers.
80-
vi.mock("./diffs/AnnotatableCodeView", () => ({ AnnotatableCodeView: () => null }));
8187
vi.mock("./diffs/DiffFileTree", () => ({ DiffFileTree: () => null }));
8288
vi.mock("./DiffFilePathCopyButton", () => ({ DiffFilePathCopyButton: () => null }));
8389

@@ -118,6 +124,11 @@ function setup(failedBaseRef: string | null) {
118124
useDiffPanelSessionStore.setState({ collapsedFileKeysByScopeKey: {} });
119125
};
120126
reset();
127+
state.diffFilesCollapsed = false;
128+
const patch = Atom.make<AsyncResult.AsyncResult<ReviewDiffPreviewSource, Error>>(
129+
AsyncResult.initial(),
130+
);
131+
state.patch = patch;
121132
state.threadRef = {
122133
environmentId: EnvironmentId.make("environment"),
123134
threadId: ThreadId.make("thread"),
@@ -169,6 +180,7 @@ function setup(failedBaseRef: string | null) {
169180
const registry = AtomRegistry.make();
170181
return {
171182
registry,
183+
patch,
172184
threadRef: state.threadRef,
173185
[Symbol.dispose]() {
174186
registry.dispose();
@@ -214,3 +226,43 @@ it.each([
214226
});
215227
},
216228
);
229+
230+
it.each([false, true])(
231+
"expands an unloaded file on the first click with default collapse %s",
232+
async (collapsed) => {
233+
using context = setup("origin/orphan");
234+
state.diffFilesCollapsed = collapsed;
235+
const path = `pending-${collapsed}.txt`;
236+
const metadata = preview();
237+
const source = {
238+
...metadata.sources[0]!,
239+
files: [{ path, previousPath: null, additions: 1, deletions: 1 }],
240+
};
241+
state.previews.set("main", Atom.make(AsyncResult.success({ ...metadata, sources: [source] })));
242+
useDiffPanelStore.getState().selectBranchBaseRef(context.threadRef, "main");
243+
await using screen = await render(
244+
<RegistryContext.Provider value={context.registry}>
245+
<div style={{ height: 600, display: "flex", flexDirection: "column" }}>
246+
<DiffPanel composerDraftTarget={context.threadRef} workspaceMutationId={null} visible />
247+
</div>
248+
</RegistryContext.Provider>,
249+
);
250+
const toggle = screen.getByRole("button", { name: `Expand ${path}`, exact: true });
251+
await expect.element(toggle).toHaveAttribute("aria-expanded", "false");
252+
await userEvent.click(toggle);
253+
act(() =>
254+
context.registry.set(
255+
context.patch,
256+
AsyncResult.success({
257+
...source,
258+
diff: `diff --git a/${path} b/${path}\n--- a/${path}\n+++ b/${path}\n@@ -1 +1 @@\n-before\n+after\n`,
259+
}),
260+
),
261+
);
262+
const loadedToggle = screen.getByRole("button", { name: `Collapse ${path}`, exact: true });
263+
await expect.element(loadedToggle).toHaveAttribute("aria-expanded", "true");
264+
await expect.element(screen.getByText("after", { exact: true })).toBeVisible();
265+
await userEvent.click(loadedToggle);
266+
await expect.element(toggle).toHaveAttribute("aria-expanded", "false");
267+
},
268+
);

‎apps/web/src/components/DiffPanel.tsx‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ function DiffFileCollapseToggle({
156156
aria-label={collapsed ? `Expand ${filePath}` : `Collapse ${filePath}`}
157157
aria-expanded={!collapsed}
158158
ref={(node) => {
159-
if (!node || !load || collapsed) return;
159+
if (!node || !load) return;
160160
return observeIntersection(
161161
node,
162162
(entry) => {
@@ -750,9 +750,10 @@ export default function DiffPanel({
750750
const toggleDiffFileCollapsed = useStableCallback((fileKey: string) => {
751751
const file = renderableFileEntries.find((file) => file.fileKey === fileKey);
752752
if (lazySource && file) requestFile(resolveFileDiffPath(file.fileDiff));
753+
const pending = file?.fileDiff.cacheKey?.endsWith(":pending") === true;
753754
updateCollapsedDiffFiles((current) => {
754755
const next = new Set(current);
755-
if (next.has(fileKey)) {
756+
if (pending || next.has(fileKey)) {
756757
next.delete(fileKey);
757758
} else {
758759
next.add(fileKey);
@@ -1462,14 +1463,15 @@ export default function DiffPanel({
14621463
}
14631464
: {})}
14641465
onRevealSearchMatch={unfoldDiffFile}
1465-
renderHeaderPrefix={(fileDiff, fileKey) => {
1466+
renderHeaderPrefix={(fileDiff, fileKey, collapsed) => {
14661467
return (
14671468
<DiffFileCollapseToggle
14681469
filePath={resolveFileDiffPath(fileDiff)}
14691470
fileKey={fileKey}
1470-
collapsed={collapsedDiffFileKeys.has(fileKey)}
1471+
collapsed={collapsed}
14711472
onToggle={toggleDiffFileCollapsed}
1472-
{...(fileDiff.cacheKey?.endsWith(":pending")
1473+
{...(fileDiff.cacheKey?.endsWith(":pending") &&
1474+
!collapsedDiffFileKeys.has(fileKey)
14731475
? {
14741476
load: () => requestFile(resolveFileDiffPath(fileDiff)),
14751477
}

0 commit comments

Comments
 (0)