fix/improve terminal hook (#1371)

frontend/package-lock.json

@@ -13,6 +13,7 @@
         "@react-types/shared": "^3.22.1",
         "@reduxjs/toolkit": "^2.2.2",
         "@vitejs/plugin-react": "^4.2.1",
+        "@xterm/addon-fit": "^0.10.0",
         "@xterm/xterm": "^5.4.0",
         "clsx": "^2.1.0",
         "eslint-config-airbnb-typescript": "^18.0.0",
@@ -32,8 +33,7 @@
         "react-syntax-highlighter": "^15.5.0",
         "tailwind-merge": "^2.2.2",
         "vite": "^5.1.6",
-        "web-vitals": "^2.1.4",
-        "xterm-addon-fit": "^0.8.0"
+        "web-vitals": "^2.1.4"
       },
       "devDependencies": {
         "@testing-library/jest-dom": "^6.4.2",
@@ -5247,6 +5247,14 @@
       "integrity": "sha512-xWGDIW6x921xtzPkhiULtthJHoJvBbF3q26fzloPCK0hsvxtPVelvftw3zjbHWSkR2km9Z+4uxbDDK/6Zw9B8w==",
       "dev": true
     },
+    "node_modules/@xterm/addon-fit": {
+      "version": "0.10.0",
+      "resolved": "https://registry.npmjs.org/@xterm/addon-fit/-/addon-fit-0.10.0.tgz",
+      "integrity": "sha512-UFYkDm4HUahf2lnEyHvio51TNGiLK66mqP2JoATy7hRZeXaGMRDr00JiSF7m63vR5WKATF605yEggJKsw0JpMQ==",
+      "peerDependencies": {
+        "@xterm/xterm": "^5.0.0"
+      }
+    },
     "node_modules/@xterm/xterm": {
       "version": "5.5.0",
       "resolved": "https://registry.npmjs.org/@xterm/xterm/-/xterm-5.5.0.tgz",
@@ -14260,22 +14268,6 @@
         "node": ">=0.4"
       }
     },
-    "node_modules/xterm": {
-      "version": "5.3.0",
-      "resolved": "https://registry.npmjs.org/xterm/-/xterm-5.3.0.tgz",
-      "integrity": "sha512-8QqjlekLUFTrU6x7xck1MsPzPA571K5zNqWm0M0oroYEWVOptZ0+ubQSkQ3uxIEhcIHRujJy6emDWX4A7qyFzg==",
-      "deprecated": "This package is now deprecated. Move to @xterm/xterm instead.",
-      "peer": true
-    },
-    "node_modules/xterm-addon-fit": {
-      "version": "0.8.0",
-      "resolved": "https://registry.npmjs.org/xterm-addon-fit/-/xterm-addon-fit-0.8.0.tgz",
-      "integrity": "sha512-yj3Np7XlvxxhYF/EJ7p3KHaMt6OdwQ+HDu573Vx1lRXsVxOcnVJs51RgjZOouIZOczTsskaS+CpXspK81/DLqw==",
-      "deprecated": "This package is now deprecated. Move to @xterm/addon-fit instead.",
-      "peerDependencies": {
-        "xterm": "^5.0.0"
-      }
-    },
     "node_modules/y18n": {
       "version": "5.0.8",
       "resolved": "https://registry.npmjs.org/y18n/-/y18n-5.0.8.tgz",

frontend/package.json

@@ -12,6 +12,7 @@
     "@react-types/shared": "^3.22.1",
     "@reduxjs/toolkit": "^2.2.2",
     "@vitejs/plugin-react": "^4.2.1",
+    "@xterm/addon-fit": "^0.10.0",
     "@xterm/xterm": "^5.4.0",
     "clsx": "^2.1.0",
     "eslint-config-airbnb-typescript": "^18.0.0",
@@ -31,8 +32,7 @@
     "react-syntax-highlighter": "^15.5.0",
     "tailwind-merge": "^2.2.2",
     "vite": "^5.1.6",
-    "web-vitals": "^2.1.4",
-    "xterm-addon-fit": "^0.8.0"
+    "web-vitals": "^2.1.4"
   },
   "scripts": {
     "start": "vite",

frontend/src/App.tsx

@@ -15,7 +15,7 @@ import { ResFetchMsgTotal } from "#/types/ResponseType";
 import "./App.css";
 import AgentControlBar from "./components/AgentControlBar";
 import AgentStatusBar from "./components/AgentStatusBar";
-import Terminal from "./components/Terminal";
+import Terminal from "./components/terminal/Terminal";
 
 interface Props {
   setSettingOpen: (isOpen: boolean) => void;

frontend/src/components/Terminal.tsx

@@ -1,115 +0,0 @@
-import { IDisposable, Terminal as XtermTerminal } from "@xterm/xterm";
-import "@xterm/xterm/css/xterm.css";
-import React, { useEffect, useRef } from "react";
-import { VscTerminal } from "react-icons/vsc";
-import { useSelector } from "react-redux";
-import { FitAddon } from "xterm-addon-fit";
-import Socket from "#/services/socket";
-import { RootState } from "#/store";
-import ActionType from "#/types/ActionType";
-import ObservationType from "#/types/ObservationType";
-
-class JsonWebsocketAddon {
-  _disposables: IDisposable[];
-
-  constructor() {
-    this._disposables = [];
-  }
-
-  activate(terminal: XtermTerminal) {
-    this._disposables.push(
-      terminal.onData((data) => {
-        const payload = JSON.stringify({ action: "terminal", data });
-        Socket.send(payload);
-      }),
-    );
-    Socket.addEventListener("message", (event) => {
-      const { action, args, observation, content } = JSON.parse(event.data);
-      if (action === ActionType.RUN) {
-        terminal.writeln(args.command);
-      }
-      if (observation === ObservationType.RUN) {
-        content.split("\n").forEach((line: string) => {
-          terminal.writeln(line);
-        });
-        terminal.write("\n$ ");
-      }
-    });
-  }
-
-  dispose() {
-    this._disposables.forEach((d) => d.dispose());
-    Socket.removeEventListener("message", () => {});
-  }
-}
-
-/**
- * The terminal's content is set by write messages. To avoid complicated state logic,
- * we keep the terminal persistently open as a child of <App /> and hidden when not in use.
- */
-
-function Terminal(): JSX.Element {
-  const terminalRef = useRef<HTMLDivElement>(null);
-  const { commands } = useSelector((state: RootState) => state.cmd);
-
-  useEffect(() => {
-    const terminal = new XtermTerminal({
-      // This value is set to the appropriate value by the
-      // `fitAddon.fit()` call below.
-      // If not set here, the terminal does not respect the width
-      // of its parent element. This causes a bug where the terminal
-      // is too large and switching tabs causes a layout shift.
-      cols: 0,
-      fontFamily: "Menlo, Monaco, 'Courier New', monospace",
-      fontSize: 14,
-      theme: {
-        background: "#262626",
-      },
-    });
-    terminal.write("$ ");
-
-    const fitAddon = new FitAddon();
-    terminal.loadAddon(fitAddon);
-
-    terminal.open(terminalRef.current as HTMLDivElement);
-
-    // Without this timeout, `fitAddon.fit()` throws the error
-    // "this._renderer.value is undefined"
-    setTimeout(() => {
-      fitAddon.fit();
-    }, 1);
-
-    const jsonWebsocketAddon = new JsonWebsocketAddon();
-    terminal.loadAddon(jsonWebsocketAddon);
-
-    // FIXME, temporary solution to display the terminal,
-    // but it will rerender the terminal every time the commands change
-    commands.forEach((command) => {
-      if (command.type === "input") {
-        terminal.writeln(command.content);
-      } else {
-        command.content.split("\n").forEach((line: string) => {
-          terminal.writeln(line);
-        });
-        terminal.write("\n$ ");
-      }
-    });
-    return () => {
-      terminal.dispose();
-    };
-  }, [commands]);
-
-  return (
-    <div className="flex flex-col h-full">
-      <div className="flex items-center gap-2 px-4 py-2 text-sm border-b border-neutral-600">
-        <VscTerminal />
-        Terminal
-      </div>
-      <div className="grow p-2 flex min-h-0">
-        <div ref={terminalRef} className="h-full w-full" />
-      </div>
-    </div>
-  );
-}
-
-export default Terminal;

frontend/src/components/terminal/Terminal.test.tsx

@@ -0,0 +1,116 @@
+import React from "react";
+import { act, screen } from "@testing-library/react";
+import { renderWithProviders } from "test-utils";
+import { Command, appendInput, appendOutput } from "#/state/commandSlice";
+import Terminal from "./Terminal";
+
+global.ResizeObserver = vi.fn().mockImplementation(() => ({
+  observe: vi.fn(),
+  disconnect: vi.fn(),
+}));
+
+const openMock = vi.fn();
+const writeMock = vi.fn();
+const writelnMock = vi.fn();
+const disposeMock = vi.fn();
+
+vi.mock("@xterm/xterm", async (importOriginal) => ({
+  ...(await importOriginal<typeof import("@xterm/xterm")>()),
+  Terminal: vi.fn(() => ({
+    open: openMock,
+    write: writeMock,
+    writeln: writelnMock,
+    dispose: disposeMock,
+    loadAddon: vi.fn(),
+  })),
+}));
+
+const renderTerminal = (commands: Command[] = []) =>
+  renderWithProviders(<Terminal />, {
+    preloadedState: {
+      cmd: {
+        commands,
+      },
+    },
+  });
+
+describe("Terminal", () => {
+  afterEach(() => {
+    vi.clearAllMocks();
+  });
+
+  it("should render a terminal", () => {
+    renderTerminal();
+
+    expect(screen.getByText("Terminal (read-only)")).toBeInTheDocument();
+    expect(openMock).toHaveBeenCalledTimes(1);
+
+    expect(writeMock).toHaveBeenCalledWith("$ ");
+  });
+
+  it("should load commands to the terminal", () => {
+    renderTerminal([
+      { type: "input", content: "INPUT" },
+      { type: "output", content: "OUTPUT" },
+    ]);
+
+    expect(writelnMock).toHaveBeenNthCalledWith(1, "INPUT");
+    expect(writelnMock).toHaveBeenNthCalledWith(2, "OUTPUT");
+  });
+
+  it("should write commands to the terminal", () => {
+    const { store } = renderTerminal();
+
+    act(() => {
+      store.dispatch(appendInput("echo Hello"));
+      store.dispatch(appendOutput("Hello"));
+    });
+
+    expect(writelnMock).toHaveBeenNthCalledWith(1, "echo Hello");
+    expect(writelnMock).toHaveBeenNthCalledWith(2, "Hello");
+
+    act(() => {
+      store.dispatch(appendInput("echo World"));
+    });
+
+    expect(writelnMock).toHaveBeenNthCalledWith(3, "echo World");
+  });
+
+  it("should load and write commands to the terminal", () => {
+    const { store } = renderTerminal([
+      { type: "input", content: "echo Hello" },
+      { type: "output", content: "Hello" },
+    ]);
+
+    expect(writelnMock).toHaveBeenNthCalledWith(1, "echo Hello");
+    expect(writelnMock).toHaveBeenNthCalledWith(2, "Hello");
+
+    act(() => {
+      store.dispatch(appendInput("echo Hello"));
+    });
+
+    expect(writelnMock).toHaveBeenNthCalledWith(3, "echo Hello");
+  });
+
+  it("should end the line with a dollar sign after writing a command", () => {
+    const { store } = renderTerminal();
+
+    act(() => {
+      store.dispatch(appendInput("echo Hello"));
+    });
+
+    expect(writelnMock).toHaveBeenCalledWith("echo Hello");
+    expect(writeMock).toHaveBeenCalledWith("$ ");
+  });
+
+  // This test fails because it expects `disposeMock` to have been called before the component is unmounted.
+  it.skip("should dispose the terminal on unmount", () => {
+    const { unmount } = renderWithProviders(<Terminal />);
+
+    expect(disposeMock).not.toHaveBeenCalled();
+
+    unmount();
+
+    expect(disposeMock).toHaveBeenCalledTimes(1);
+  });
+});

frontend/src/components/terminal/Terminal.tsx

@@ -0,0 +1,26 @@
+import React from "react";
+import { useSelector } from "react-redux";
+import { VscTerminal } from "react-icons/vsc";
+import { RootState } from "#/store";
+import { useTerminal } from "../../hooks/useTerminal";
+
+import "@xterm/xterm/css/xterm.css";
+
+function Terminal() {
+  const { commands } = useSelector((state: RootState) => state.cmd);
+  const ref = useTerminal(commands);
+
+  return (
+    <div className="flex flex-col h-full">
+      <div className="flex items-center gap-2 px-4 py-2 text-sm border-b border-neutral-600">
+        <VscTerminal />
+        Terminal (read-only)
+      </div>
+      <div className="grow p-2 flex min-h-0">
+        <div ref={ref} className="h-full w-full" />
+      </div>
+    </div>
+  );
+}
+
+export default Terminal;

frontend/src/hooks/useTerminal.ts

@@ -0,0 +1,72 @@
+import { FitAddon } from "@xterm/addon-fit";
+import { Terminal } from "@xterm/xterm";
+import React from "react";
+import { Command } from "#/state/commandSlice";
+
+/*
+  NOTE: Tests for this hook are indirectly covered by the tests for the XTermTerminal component.
+  The reason for this is that the hook exposes a ref that requires a DOM element to be rendered.
+*/
+
+export const useTerminal = (commands: Command[] = []) => {
+  const terminal = React.useRef<Terminal | null>(null);
+  const fitAddon = React.useRef<FitAddon | null>(null);
+  const ref = React.useRef<HTMLDivElement>(null);
+  const lastCommandIndex = React.useRef(0);
+
+  React.useEffect(() => {
+    /* Create a new terminal instance */
+    terminal.current = new Terminal({
+      fontFamily: "Menlo, Monaco, 'Courier New', monospace",
+      fontSize: 14,
+      theme: {
+        background: "#262626",
+      },
+    });
+    fitAddon.current = new FitAddon();
+
+    let resizeObserver: ResizeObserver;
+
+    if (ref.current) {
+      /* Initialize the terminal in the DOM */
+      terminal.current.loadAddon(fitAddon.current);
+      terminal.current.open(ref.current);
+
+      terminal.current.write("$ ");
+
+      /* Listen for resize events */
+      resizeObserver = new ResizeObserver(() => {
+        fitAddon.current?.fit();
+      });
+      resizeObserver.observe(ref.current);
+    }
+
+    return () => {
+      terminal.current?.dispose();
+      resizeObserver.disconnect();
+    };
+  }, []);
+
+  React.useEffect(() => {
+    /* Write commands to the terminal */
+    if (terminal.current && commands.length > 0) {
+      // Start writing commands from the last command index
+      for (let i = lastCommandIndex.current; i < commands.length; i += 1) {
+        const command = commands[i];
+        const lines = command.content.split("\n");
+
+        lines.forEach((line: string) => {
+          terminal.current?.writeln(line);
+        });
+
+        if (command.type === "output") {
+          terminal.current.write("\n$ ");
+        }
+      }
+
+      lastCommandIndex.current = commands.length; // Update the position of the last command
+    }
+  }, [commands]);
+
+  return ref;
+};