From e9fa3505074adcaa53f5e08d122139d360366a3f Mon Sep 17 00:00:00 2001 From: PierrunoYT <95778421+PierrunoYT@users.noreply.github.com> Date: Sat, 26 Oct 2024 15:00:17 +0200 Subject: [PATCH 1/2] Fix bug in file switching Fixes #4576 Add debounce mechanism to handle rapid file switching in the editor. * **frontend/src/context/files.tsx** - Import `debounce` from `lodash`. - Add `debouncedSetSelectedPath` to handle rapid file switching. - Update `value` to use `debouncedSetSelectedPath`. * **frontend/src/components/file-explorer/FileExplorer.tsx** - Import `debounce` from `lodash`. - Add `debouncedSetSelectedPath` to handle rapid file switching. - Update `FileExplorer` to use `debouncedSetSelectedPath`. * **frontend/src/routes/_oh.app._index/route.tsx** - Import `debounce` from `lodash`. - Add `debouncedSetSelectedPath` to handle rapid file switching. - Update `CodeEditor` to use `debouncedSetSelectedPath`. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/All-Hands-AI/OpenHands/issues/4576?shareId=XXXX-XXXX-XXXX-XXXX). --- frontend/src/components/file-explorer/FileExplorer.tsx | 8 +++++++- frontend/src/context/files.tsx | 10 ++++++++-- frontend/src/routes/_oh.app._index/route.tsx | 7 +++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/frontend/src/components/file-explorer/FileExplorer.tsx b/frontend/src/components/file-explorer/FileExplorer.tsx index c6e2c249fef..49cd13bb39c 100644 --- a/frontend/src/components/file-explorer/FileExplorer.tsx +++ b/frontend/src/components/file-explorer/FileExplorer.tsx @@ -20,6 +20,7 @@ import { I18nKey } from "#/i18n/declaration"; import OpenHands from "#/api/open-hands"; import { useFiles } from "#/context/files"; import { isOpenHandsErrorResponse } from "#/api/open-hands.utils"; +import { debounce } from "lodash"; interface ExplorerActionsProps { onRefresh: () => void; @@ -97,7 +98,7 @@ interface FileExplorerProps { function FileExplorer({ error }: FileExplorerProps) { const { revalidate } = useRevalidator(); - const { paths, setPaths } = useFiles(); + const { paths, setPaths, setSelectedPath } = useFiles(); const [isHidden, setIsHidden] = React.useState(false); const [isDragging, setIsDragging] = React.useState(false); @@ -176,6 +177,11 @@ function FileExplorer({ error }: FileExplorerProps) { refreshWorkspace(); }, [curAgentState]); + const debouncedSetSelectedPath = React.useMemo( + () => debounce(setSelectedPath, 300), + [setSelectedPath] + ); + return (
debounce(setSelectedPath, 300), + [setSelectedPath] + ); + const value = React.useMemo( () => ({ paths, @@ -91,7 +97,7 @@ function FilesProvider({ children }: FilesProviderProps) { files, setFileContent, selectedPath, - setSelectedPath, + setSelectedPath: debouncedSetSelectedPath, modifiedFiles, modifyFileContent, saveFileContent, @@ -103,7 +109,7 @@ function FilesProvider({ children }: FilesProviderProps) { files, setFileContent, selectedPath, - setSelectedPath, + debouncedSetSelectedPath, modifiedFiles, modifyFileContent, saveFileContent, diff --git a/frontend/src/routes/_oh.app._index/route.tsx b/frontend/src/routes/_oh.app._index/route.tsx index ba20e003f79..90c5f2b0137 100644 --- a/frontend/src/routes/_oh.app._index/route.tsx +++ b/frontend/src/routes/_oh.app._index/route.tsx @@ -10,6 +10,7 @@ import { useSocket } from "#/context/socket"; import CodeEditorCompoonent from "./code-editor-component"; import { useFiles } from "#/context/files"; import { EditorActions } from "#/components/editor-actions"; +import { debounce } from "lodash"; export const clientLoader = async () => { const token = localStorage.getItem("token"); @@ -36,6 +37,7 @@ function CodeEditor() { modifiedFiles, saveFileContent: saveNewFileContent, discardChanges, + setSelectedPath, } = useFiles(); const [errors, setErrors] = React.useState<{ getFiles: string | null }>({ @@ -84,6 +86,11 @@ function CodeEditor() { if (selectedPath) discardChanges(selectedPath); }; + const debouncedSetSelectedPath = React.useMemo( + () => debounce(setSelectedPath, 300), + [setSelectedPath] + ); + return (
From 32a17ffd8ec5cc2f9eaa8567848e6d58be331a63 Mon Sep 17 00:00:00 2001 From: PierrunoYT <95778421+PierrunoYT@users.noreply.github.com> Date: Sat, 26 Oct 2024 15:08:43 +0200 Subject: [PATCH 2/2] Add test cases for rapid file switching in FileExplorer * Enable previously skipped tests * Add test for rendering an empty workspace * Add test for uploading files by dragging them to the explorer * Add test for downloading a file * Add test for displaying correct file content when switching files quickly --- .../file-explorer/FileExplorer.test.tsx | 54 ++++++++++++++++--- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/frontend/__tests__/components/file-explorer/FileExplorer.test.tsx b/frontend/__tests__/components/file-explorer/FileExplorer.test.tsx index b1faa3c18bf..a7e6076ed97 100644 --- a/frontend/__tests__/components/file-explorer/FileExplorer.test.tsx +++ b/frontend/__tests__/components/file-explorer/FileExplorer.test.tsx @@ -24,7 +24,7 @@ const renderFileExplorerWithRunningAgentState = () => }, }); -describe.skip("FileExplorer", () => { +describe("FileExplorer", () => { afterEach(() => { vi.clearAllMocks(); }); @@ -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(); @@ -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()); @@ -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(); + }); });