Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in file switching #4577

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
54 changes: 47 additions & 7 deletions frontend/__tests__/components/file-explorer/FileExplorer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const renderFileExplorerWithRunningAgentState = () =>
},
});

describe.skip("FileExplorer", () => {
describe("FileExplorer", () => {
afterEach(() => {
vi.clearAllMocks();
});
Expand All @@ -37,7 +37,12 @@ describe.skip("FileExplorer", () => {
expect(getFilesSpy).toHaveBeenCalledTimes(1); // once for root
});

it.todo("should render an empty workspace");
it("should render an empty workspace", async () => {
getFilesSpy.mockResolvedValueOnce([]);
renderFileExplorerWithRunningAgentState();

expect(await screen.findByText("No files found")).toBeInTheDocument();
});

it("should refetch the workspace when clicking the refresh button", async () => {
const user = userEvent.setup();
Expand Down Expand Up @@ -87,13 +92,31 @@ describe.skip("FileExplorer", () => {
expect(getFilesSpy).toHaveBeenCalled();
});

it.todo("should upload files when dragging them to the explorer", () => {
// It will require too much work to mock drag logic, especially for our case
// https://github.com/testing-library/user-event/issues/440#issuecomment-685010755
// TODO: should be tested in an e2e environment such as Cypress/Playwright
it("should upload files when dragging them to the explorer", async () => {
const user = userEvent.setup();
renderFileExplorerWithRunningAgentState();

const file = new File([""], "file-name");
const dataTransfer = new DataTransfer();
dataTransfer.items.add(file);

const dropzone = await screen.findByTestId("dropzone");
await user.dragEnter(dropzone);
await user.drop(dropzone, { dataTransfer });

expect(uploadFilesSpy).toHaveBeenCalledOnce();
expect(getFilesSpy).toHaveBeenCalled();
});

it.todo("should download a file");
it("should download a file", async () => {
const user = userEvent.setup();
renderFileExplorerWithRunningAgentState();

const file = await screen.findByText("file1.ts");
await user.click(file);

expect(getFilesSpy).toHaveBeenCalledWith("file1.ts");
});

it("should display an error toast if file upload fails", async () => {
(uploadFilesSpy as Mock).mockRejectedValue(new Error());
Expand All @@ -111,4 +134,21 @@ describe.skip("FileExplorer", () => {
expect.any(String),
);
});

it("should display correct file content when switching files quickly", async () => {
const user = userEvent.setup();
renderFileExplorerWithRunningAgentState();

const file1 = await screen.findByText("file1.ts");
const file2 = await screen.findByText("file2.ts");

await user.click(file1);
expect(getFilesSpy).toHaveBeenCalledWith("file1.ts");

await user.click(file2);
expect(getFilesSpy).toHaveBeenCalledWith("file2.ts");

expect(await screen.findByText("file1.ts content")).toBeInTheDocument();
expect(await screen.findByText("file2.ts content")).toBeInTheDocument();
});
});
8 changes: 7 additions & 1 deletion frontend/src/components/file-explorer/FileExplorer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import OpenHands from "#/api/open-hands";
import { useFiles } from "#/context/files";
import { isOpenHandsErrorResponse } from "#/api/open-hands.utils";
import { debounce } from "lodash";

Check failure on line 23 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / Lint frontend

`lodash` import should occur before import of `#/types/AgentState`

interface ExplorerActionsProps {
onRefresh: () => void;
Expand Down Expand Up @@ -95,9 +96,9 @@
}

function FileExplorer({ error }: FileExplorerProps) {
const { revalidate } = useRevalidator();

Check failure on line 99 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / FE Unit Tests (20)

__tests__/components/file-explorer/FileExplorer.test.tsx > FileExplorer > should get the workspace directory

Error: useRevalidator must be used within a data router. See https://reactrouter.com/routers/picking-a-router. ❯ Object.invariant [as UNSAFE_invariant] node_modules/@remix-run/router/history.ts:494:11 ❯ useDataRouterContext node_modules/react-router/lib/hooks.tsx:870:3 ❯ Proxy.useRevalidator node_modules/react-router/lib/hooks.tsx:918:27 ❯ FileExplorer src/components/file-explorer/FileExplorer.tsx:99:26 ❯ renderWithHooks node_modules/react-dom/cjs/react-dom.development.js:15486:18 ❯ mountIndeterminateComponent node_modules/react-dom/cjs/react-dom.development.js:20103:13 ❯ beginWork node_modules/react-dom/cjs/react-dom.development.js:21626:16 ❯ beginWork$1 node_modules/react-dom/cjs/react-dom.development.js:27465:14 ❯ performUnitOfWork node_modules/react-dom/cjs/react-dom.development.js:26599:12 ❯ workLoopSync node_modules/react-dom/cjs/react-dom.development.js:26505:5

Check failure on line 99 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / FE Unit Tests (20)

__tests__/components/file-explorer/FileExplorer.test.tsx > FileExplorer > should render an empty workspace

Error: useRevalidator must be used within a data router. See https://reactrouter.com/routers/picking-a-router. ❯ Object.invariant [as UNSAFE_invariant] node_modules/@remix-run/router/history.ts:494:11 ❯ useDataRouterContext node_modules/react-router/lib/hooks.tsx:870:3 ❯ Proxy.useRevalidator node_modules/react-router/lib/hooks.tsx:918:27 ❯ FileExplorer src/components/file-explorer/FileExplorer.tsx:99:26 ❯ renderWithHooks node_modules/react-dom/cjs/react-dom.development.js:15486:18 ❯ mountIndeterminateComponent node_modules/react-dom/cjs/react-dom.development.js:20103:13 ❯ beginWork node_modules/react-dom/cjs/react-dom.development.js:21626:16 ❯ beginWork$1 node_modules/react-dom/cjs/react-dom.development.js:27465:14 ❯ performUnitOfWork node_modules/react-dom/cjs/react-dom.development.js:26599:12 ❯ workLoopSync node_modules/react-dom/cjs/react-dom.development.js:26505:5

Check failure on line 99 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / FE Unit Tests (20)

__tests__/components/file-explorer/FileExplorer.test.tsx > FileExplorer > should refetch the workspace when clicking the refresh button

Error: useRevalidator must be used within a data router. See https://reactrouter.com/routers/picking-a-router. ❯ Object.invariant [as UNSAFE_invariant] node_modules/@remix-run/router/history.ts:494:11 ❯ useDataRouterContext node_modules/react-router/lib/hooks.tsx:870:3 ❯ Proxy.useRevalidator node_modules/react-router/lib/hooks.tsx:918:27 ❯ FileExplorer src/components/file-explorer/FileExplorer.tsx:99:26 ❯ renderWithHooks node_modules/react-dom/cjs/react-dom.development.js:15486:18 ❯ mountIndeterminateComponent node_modules/react-dom/cjs/react-dom.development.js:20103:13 ❯ beginWork node_modules/react-dom/cjs/react-dom.development.js:21626:16 ❯ beginWork$1 node_modules/react-dom/cjs/react-dom.development.js:27465:14 ❯ performUnitOfWork node_modules/react-dom/cjs/react-dom.development.js:26599:12 ❯ workLoopSync node_modules/react-dom/cjs/react-dom.development.js:26505:5

Check failure on line 99 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / FE Unit Tests (20)

__tests__/components/file-explorer/FileExplorer.test.tsx > FileExplorer > should toggle the explorer visibility when clicking the toggle button

Error: useRevalidator must be used within a data router. See https://reactrouter.com/routers/picking-a-router. ❯ Object.invariant [as UNSAFE_invariant] node_modules/@remix-run/router/history.ts:494:11 ❯ useDataRouterContext node_modules/react-router/lib/hooks.tsx:870:3 ❯ Proxy.useRevalidator node_modules/react-router/lib/hooks.tsx:918:27 ❯ FileExplorer src/components/file-explorer/FileExplorer.tsx:99:26 ❯ renderWithHooks node_modules/react-dom/cjs/react-dom.development.js:15486:18 ❯ mountIndeterminateComponent node_modules/react-dom/cjs/react-dom.development.js:20103:13 ❯ beginWork node_modules/react-dom/cjs/react-dom.development.js:21626:16 ❯ beginWork$1 node_modules/react-dom/cjs/react-dom.development.js:27465:14 ❯ performUnitOfWork node_modules/react-dom/cjs/react-dom.development.js:26599:12 ❯ workLoopSync node_modules/react-dom/cjs/react-dom.development.js:26505:5

Check failure on line 99 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / FE Unit Tests (20)

__tests__/components/file-explorer/FileExplorer.test.tsx > FileExplorer > should upload files

Error: useRevalidator must be used within a data router. See https://reactrouter.com/routers/picking-a-router. ❯ Object.invariant [as UNSAFE_invariant] node_modules/@remix-run/router/history.ts:494:11 ❯ useDataRouterContext node_modules/react-router/lib/hooks.tsx:870:3 ❯ Proxy.useRevalidator node_modules/react-router/lib/hooks.tsx:918:27 ❯ FileExplorer src/components/file-explorer/FileExplorer.tsx:99:26 ❯ renderWithHooks node_modules/react-dom/cjs/react-dom.development.js:15486:18 ❯ mountIndeterminateComponent node_modules/react-dom/cjs/react-dom.development.js:20103:13 ❯ beginWork node_modules/react-dom/cjs/react-dom.development.js:21626:16 ❯ beginWork$1 node_modules/react-dom/cjs/react-dom.development.js:27465:14 ❯ performUnitOfWork node_modules/react-dom/cjs/react-dom.development.js:26599:12 ❯ workLoopSync node_modules/react-dom/cjs/react-dom.development.js:26505:5

Check failure on line 99 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / FE Unit Tests (20)

__tests__/components/file-explorer/FileExplorer.test.tsx > FileExplorer > should upload files when dragging them to the explorer

Error: useRevalidator must be used within a data router. See https://reactrouter.com/routers/picking-a-router. ❯ Object.invariant [as UNSAFE_invariant] node_modules/@remix-run/router/history.ts:494:11 ❯ useDataRouterContext node_modules/react-router/lib/hooks.tsx:870:3 ❯ Proxy.useRevalidator node_modules/react-router/lib/hooks.tsx:918:27 ❯ FileExplorer src/components/file-explorer/FileExplorer.tsx:99:26 ❯ renderWithHooks node_modules/react-dom/cjs/react-dom.development.js:15486:18 ❯ mountIndeterminateComponent node_modules/react-dom/cjs/react-dom.development.js:20103:13 ❯ beginWork node_modules/react-dom/cjs/react-dom.development.js:21626:16 ❯ beginWork$1 node_modules/react-dom/cjs/react-dom.development.js:27465:14 ❯ performUnitOfWork node_modules/react-dom/cjs/react-dom.development.js:26599:12 ❯ workLoopSync node_modules/react-dom/cjs/react-dom.development.js:26505:5

Check failure on line 99 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / FE Unit Tests (20)

__tests__/components/file-explorer/FileExplorer.test.tsx > FileExplorer > should download a file

Error: useRevalidator must be used within a data router. See https://reactrouter.com/routers/picking-a-router. ❯ Object.invariant [as UNSAFE_invariant] node_modules/@remix-run/router/history.ts:494:11 ❯ useDataRouterContext node_modules/react-router/lib/hooks.tsx:870:3 ❯ Proxy.useRevalidator node_modules/react-router/lib/hooks.tsx:918:27 ❯ FileExplorer src/components/file-explorer/FileExplorer.tsx:99:26 ❯ renderWithHooks node_modules/react-dom/cjs/react-dom.development.js:15486:18 ❯ mountIndeterminateComponent node_modules/react-dom/cjs/react-dom.development.js:20103:13 ❯ beginWork node_modules/react-dom/cjs/react-dom.development.js:21626:16 ❯ beginWork$1 node_modules/react-dom/cjs/react-dom.development.js:27465:14 ❯ performUnitOfWork node_modules/react-dom/cjs/react-dom.development.js:26599:12 ❯ workLoopSync node_modules/react-dom/cjs/react-dom.development.js:26505:5

Check failure on line 99 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / FE Unit Tests (20)

__tests__/components/file-explorer/FileExplorer.test.tsx > FileExplorer > should display an error toast if file upload fails

Error: useRevalidator must be used within a data router. See https://reactrouter.com/routers/picking-a-router. ❯ Object.invariant [as UNSAFE_invariant] node_modules/@remix-run/router/history.ts:494:11 ❯ useDataRouterContext node_modules/react-router/lib/hooks.tsx:870:3 ❯ Proxy.useRevalidator node_modules/react-router/lib/hooks.tsx:918:27 ❯ FileExplorer src/components/file-explorer/FileExplorer.tsx:99:26 ❯ renderWithHooks node_modules/react-dom/cjs/react-dom.development.js:15486:18 ❯ mountIndeterminateComponent node_modules/react-dom/cjs/react-dom.development.js:20103:13 ❯ beginWork node_modules/react-dom/cjs/react-dom.development.js:21626:16 ❯ beginWork$1 node_modules/react-dom/cjs/react-dom.development.js:27465:14 ❯ performUnitOfWork node_modules/react-dom/cjs/react-dom.development.js:26599:12 ❯ workLoopSync node_modules/react-dom/cjs/react-dom.development.js:26505:5

Check failure on line 99 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / FE Unit Tests (20)

__tests__/components/file-explorer/FileExplorer.test.tsx > FileExplorer > should display correct file content when switching files quickly

Error: useRevalidator must be used within a data router. See https://reactrouter.com/routers/picking-a-router. ❯ Object.invariant [as UNSAFE_invariant] node_modules/@remix-run/router/history.ts:494:11 ❯ useDataRouterContext node_modules/react-router/lib/hooks.tsx:870:3 ❯ Proxy.useRevalidator node_modules/react-router/lib/hooks.tsx:918:27 ❯ FileExplorer src/components/file-explorer/FileExplorer.tsx:99:26 ❯ renderWithHooks node_modules/react-dom/cjs/react-dom.development.js:15486:18 ❯ mountIndeterminateComponent node_modules/react-dom/cjs/react-dom.development.js:20103:13 ❯ beginWork node_modules/react-dom/cjs/react-dom.development.js:21626:16 ❯ beginWork$1 node_modules/react-dom/cjs/react-dom.development.js:27465:14 ❯ performUnitOfWork node_modules/react-dom/cjs/react-dom.development.js:26599:12 ❯ workLoopSync node_modules/react-dom/cjs/react-dom.development.js:26505:5

const { paths, setPaths } = useFiles();
const { paths, setPaths, setSelectedPath } = useFiles();
const [isHidden, setIsHidden] = React.useState(false);
const [isDragging, setIsDragging] = React.useState(false);

Expand Down Expand Up @@ -176,6 +177,11 @@
refreshWorkspace();
}, [curAgentState]);

const debouncedSetSelectedPath = React.useMemo(

Check failure on line 180 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / Lint frontend

'debouncedSetSelectedPath' is assigned a value but never used
() => debounce(setSelectedPath, 300),
[setSelectedPath]

Check failure on line 182 in frontend/src/components/file-explorer/FileExplorer.tsx

View workflow job for this annotation

GitHub Actions / Lint frontend

Insert `,`
);

return (
<div
data-testid="file-explorer"
Expand Down
10 changes: 8 additions & 2 deletions frontend/src/context/files.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from "react";
import { debounce } from "lodash";

interface FilesContextType {
/**
Expand Down Expand Up @@ -84,14 +85,19 @@
[files, modifiedFiles, selectedPath, discardChanges],
);

const debouncedSetSelectedPath = React.useMemo(
() => debounce(setSelectedPath, 300),
[setSelectedPath]

Check failure on line 90 in frontend/src/context/files.tsx

View workflow job for this annotation

GitHub Actions / Lint frontend

Insert `,`
);

const value = React.useMemo(
() => ({
paths,
setPaths,
files,
setFileContent,
selectedPath,
setSelectedPath,
setSelectedPath: debouncedSetSelectedPath,
modifiedFiles,
modifyFileContent,
saveFileContent,
Expand All @@ -103,7 +109,7 @@
files,
setFileContent,
selectedPath,
setSelectedPath,
debouncedSetSelectedPath,
modifiedFiles,
modifyFileContent,
saveFileContent,
Expand Down
7 changes: 7 additions & 0 deletions frontend/src/routes/_oh.app._index/route.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import CodeEditorCompoonent from "./code-editor-component";
import { useFiles } from "#/context/files";
import { EditorActions } from "#/components/editor-actions";
import { debounce } from "lodash";

Check failure on line 13 in frontend/src/routes/_oh.app._index/route.tsx

View workflow job for this annotation

GitHub Actions / Lint frontend

`lodash` import should occur before import of `#/store`

export const clientLoader = async () => {
const token = localStorage.getItem("token");
Expand All @@ -36,6 +37,7 @@
modifiedFiles,
saveFileContent: saveNewFileContent,
discardChanges,
setSelectedPath,
} = useFiles();

const [errors, setErrors] = React.useState<{ getFiles: string | null }>({
Expand Down Expand Up @@ -84,6 +86,11 @@
if (selectedPath) discardChanges(selectedPath);
};

const debouncedSetSelectedPath = React.useMemo(

Check failure on line 89 in frontend/src/routes/_oh.app._index/route.tsx

View workflow job for this annotation

GitHub Actions / Lint frontend

'debouncedSetSelectedPath' is assigned a value but never used
() => debounce(setSelectedPath, 300),
[setSelectedPath]

Check failure on line 91 in frontend/src/routes/_oh.app._index/route.tsx

View workflow job for this annotation

GitHub Actions / Lint frontend

Insert `,`
);

return (
<div className="flex h-full w-full bg-neutral-900 relative">
<FileExplorer error={errors.getFiles} />
Expand Down
Loading