From f6b12aa9941a326ce6e31a137ad8458779d516ed Mon Sep 17 00:00:00 2001 From: Jordan Docherty <20336279+jdocherty@users.noreply.github.com> Date: Sun, 20 Apr 2025 22:27:06 -0700 Subject: [PATCH] refactor(history-overlay): split into modular functions & add tests (fixes #402) (#403) ## What This PR targets #402 and refactors the `history-overlay.tsx`component to reduce cognitive complexity by splitting the `buildLists` function into smaller, focused helper functions. It also adds comprehensive test coverage to ensure the functionality remains intact. ## Why The original `buildLists` function had high cognitive complexity due to multiple nested conditionals, complex string manipulation, and mixed responsibilities. This refactor makes the code more maintainable and easier to understand while preserving all existing functionality. ## How - Split `buildLists` into focused helper functions - Added comprehensive test coverage for all functionality - Maintained existing behavior and keyboard interactions - Improved code organization and readability ## Testing All tests pass, including: - Command mode functionality - File mode functionality - Keyboard interactions - Error handling --- codex-cli/src/components/history-overlay.tsx | 176 +++++----- codex-cli/tests/history-overlay.test.tsx | 350 +++++++++++++++++++ 2 files changed, 447 insertions(+), 79 deletions(-) create mode 100644 codex-cli/tests/history-overlay.test.tsx diff --git a/codex-cli/src/components/history-overlay.tsx b/codex-cli/src/components/history-overlay.tsx index c22e22d8..9cd2d6d0 100644 --- a/codex-cli/src/components/history-overlay.tsx +++ b/codex-cli/src/components/history-overlay.tsx @@ -14,7 +14,10 @@ export default function HistoryOverlay({ items, onExit }: Props): JSX.Element { const [mode, setMode] = useState("commands"); const [cursor, setCursor] = useState(0); - const { commands, files } = useMemo(() => buildLists(items), [items]); + const { commands, files } = useMemo( + () => formatHistoryForDisplay(items), + [items], + ); const list = mode === "commands" ? commands : files; @@ -95,7 +98,7 @@ export default function HistoryOverlay({ items, onExit }: Props): JSX.Element { ); } -function buildLists(items: Array): { +function formatHistoryForDisplay(items: Array): { commands: Array; files: Array; } { @@ -103,33 +106,9 @@ function buildLists(items: Array): { const filesSet = new Set(); for (const item of items) { - if ( - item.type === "message" && - (item as unknown as { role?: string }).role === "user" - ) { - // TODO: We're ignoring images/files here. - const parts = - (item as unknown as { content?: Array }).content ?? []; - const texts: Array = []; - if (Array.isArray(parts)) { - for (const part of parts) { - if (part && typeof part === "object" && "text" in part) { - const t = (part as unknown as { text?: string }).text; - if (typeof t === "string" && t.length > 0) { - texts.push(t); - } - } - } - } - - if (texts.length > 0) { - const fullPrompt = texts.join(" "); - // Truncate very long prompts so the history view stays legible. - const truncated = - fullPrompt.length > 120 ? `${fullPrompt.slice(0, 117)}…` : fullPrompt; - commands.push(`> ${truncated}`); - } - + const userPrompt = processUserMessage(item); + if (userPrompt) { + commands.push(userPrompt); continue; } @@ -173,31 +152,7 @@ function buildLists(items: Array): { : undefined; if (cmdArray && cmdArray.length > 0) { - commands.push(cmdArray.join(" ")); - - // Heuristic for file paths in command args - for (const part of cmdArray) { - if (!part.startsWith("-") && part.includes("/")) { - filesSet.add(part); - } - } - - // Special‑case apply_patch so we can extract the list of modified files - if (cmdArray[0] === "apply_patch" || cmdArray.includes("apply_patch")) { - const patchTextMaybe = cmdArray.find((s) => - s.includes("*** Begin Patch"), - ); - if (typeof patchTextMaybe === "string") { - const lines = patchTextMaybe.split("\n"); - for (const line of lines) { - const m = line.match(/^[-+]{3} [ab]\/(.+)$/); - if (m && m[1]) { - filesSet.add(m[1]); - } - } - } - } - + commands.push(processCommandArray(cmdArray, filesSet)); continue; // We processed this as a command; no need to treat as generic tool call. } @@ -205,33 +160,96 @@ function buildLists(items: Array): { // short argument representation to give users an idea of what // happened. if (typeof toolName === "string" && toolName.length > 0) { - let summary = toolName; - - if (argsJson && typeof argsJson === "object") { - // Extract a few common argument keys to make the summary more useful - // without being overly verbose. - const interestingKeys = [ - "path", - "file", - "filepath", - "filename", - "pattern", - ]; - for (const key of interestingKeys) { - const val = (argsJson as Record)[key]; - if (typeof val === "string") { - summary += ` ${val}`; - if (val.includes("/")) { - filesSet.add(val); - } - break; - } - } - } - - commands.push(summary); + commands.push(processNonExecTool(toolName, argsJson, filesSet)); } } return { commands, files: Array.from(filesSet) }; } + +function processUserMessage(item: ResponseItem): string | null { + if ( + item.type === "message" && + (item as unknown as { role?: string }).role === "user" + ) { + // TODO: We're ignoring images/files here. + const parts = + (item as unknown as { content?: Array }).content ?? []; + const texts: Array = []; + if (Array.isArray(parts)) { + for (const part of parts) { + if (part && typeof part === "object" && "text" in part) { + const t = (part as unknown as { text?: string }).text; + if (typeof t === "string" && t.length > 0) { + texts.push(t); + } + } + } + } + + if (texts.length > 0) { + const fullPrompt = texts.join(" "); + // Truncate very long prompts so the history view stays legible. + return fullPrompt.length > 120 + ? `> ${fullPrompt.slice(0, 117)}…` + : `> ${fullPrompt}`; + } + } + return null; +} + +function processCommandArray( + cmdArray: Array, + filesSet: Set, +): string { + const cmd = cmdArray.join(" "); + + // Heuristic for file paths in command args + for (const part of cmdArray) { + if (!part.startsWith("-") && part.includes("/")) { + filesSet.add(part); + } + } + + // Special‑case apply_patch so we can extract the list of modified files + if (cmdArray[0] === "apply_patch" || cmdArray.includes("apply_patch")) { + const patchTextMaybe = cmdArray.find((s) => s.includes("*** Begin Patch")); + if (typeof patchTextMaybe === "string") { + const lines = patchTextMaybe.split("\n"); + for (const line of lines) { + const m = line.match(/^[-+]{3} [ab]\/(.+)$/); + if (m && m[1]) { + filesSet.add(m[1]); + } + } + } + } + + return cmd; +} + +function processNonExecTool( + toolName: string, + argsJson: unknown, + filesSet: Set, +): string { + let summary = toolName; + + if (argsJson && typeof argsJson === "object") { + // Extract a few common argument keys to make the summary more useful + // without being overly verbose. + const interestingKeys = ["path", "file", "filepath", "filename", "pattern"]; + for (const key of interestingKeys) { + const val = (argsJson as Record)[key]; + if (typeof val === "string") { + summary += ` ${val}`; + if (val.includes("/")) { + filesSet.add(val); + } + break; + } + } + } + + return summary; +} diff --git a/codex-cli/tests/history-overlay.test.tsx b/codex-cli/tests/history-overlay.test.tsx new file mode 100644 index 00000000..1ae2a2d2 --- /dev/null +++ b/codex-cli/tests/history-overlay.test.tsx @@ -0,0 +1,350 @@ +/* -------------------------------------------------------------------------- * + * Tests for the HistoryOverlay component and its formatHistoryForDisplay utility function + * + * The component displays a list of commands and files from the chat history. + * It supports two modes: + * - Command mode: shows all commands and user messages + * - File mode: shows all files that were touched + * + * The formatHistoryForDisplay function processes ResponseItems to extract: + * - Commands: User messages and function calls + * - Files: Paths referenced in commands or function calls + * -------------------------------------------------------------------------- */ + +import { describe, it, expect, vi } from "vitest"; +import { render } from "ink-testing-library"; +import React from "react"; +import type { + ResponseInputMessageItem, + ResponseFunctionToolCallItem, +} from "openai/resources/responses/responses.mjs"; +import HistoryOverlay from "../src/components/history-overlay"; + +// --------------------------------------------------------------------------- +// Module mocks *must* be registered *before* the module under test is imported +// so that Vitest can replace the dependency during evaluation. +// --------------------------------------------------------------------------- + +// Mock ink's useInput to capture keyboard handlers +let keyboardHandler: ((input: string, key: any) => void) | undefined; +vi.mock("ink", async () => { + const actual = await vi.importActual("ink"); + return { + ...actual, + useInput: (handler: (input: string, key: any) => void) => { + keyboardHandler = handler; + }, + }; +}); + +// --------------------------------------------------------------------------- +// Test Helpers +// --------------------------------------------------------------------------- + +function createUserMessage(content: string): ResponseInputMessageItem { + return { + type: "message", + role: "user", + id: `msg_${Math.random().toString(36).slice(2)}`, + content: [{ type: "input_text", text: content }], + }; +} + +function createFunctionCall( + name: string, + args: unknown, +): ResponseFunctionToolCallItem { + return { + type: "function_call", + name, + id: `fn_${Math.random().toString(36).slice(2)}`, + call_id: `call_${Math.random().toString(36).slice(2)}`, + arguments: JSON.stringify(args), + }; +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe("HistoryOverlay", () => { + describe("command mode", () => { + it("displays user messages", () => { + const items = [createUserMessage("hello"), createUserMessage("world")]; + const { lastFrame } = render( + , + ); + const frame = lastFrame(); + expect(frame).toContain("hello"); + expect(frame).toContain("world"); + }); + + it("displays shell commands", () => { + const items = [ + createFunctionCall("shell", { cmd: ["ls", "-la"] }), + createFunctionCall("shell", { cmd: ["pwd"] }), + ]; + const { lastFrame } = render( + , + ); + const frame = lastFrame(); + expect(frame).toContain("ls -la"); + expect(frame).toContain("pwd"); + }); + + it("displays file operations", () => { + const items = [createFunctionCall("read_file", { path: "test.txt" })]; + const { lastFrame } = render( + , + ); + const frame = lastFrame(); + expect(frame).toContain("read_file test.txt"); + }); + + it("displays patch operations", () => { + const items = [ + createFunctionCall("shell", { + cmd: [ + "apply_patch", + "*** Begin Patch\n--- a/src/file1.txt\n+++ b/src/file1.txt\n@@ -1,5 +1,5 @@\n-const x = 1;\n+const x = 2;\n", + ], + }), + ]; + const { lastFrame } = render( + , + ); + + // Verify patch is displayed in command mode + let frame = lastFrame(); + expect(frame).toContain("apply_patch"); + expect(frame).toContain("src/file1.txt"); + + // Verify file is extracted in file mode + keyboardHandler?.("f", {}); + frame = lastFrame(); + expect(frame).toContain("src/file1.txt"); + }); + + it("displays mixed content in chronological order", () => { + const items = [ + createUserMessage("first message"), + createFunctionCall("shell", { cmd: ["echo", "hello"] }), + createUserMessage("second message"), + ]; + const { lastFrame } = render( + , + ); + const frame = lastFrame(); + expect(frame).toContain("first message"); + expect(frame).toContain("echo hello"); + expect(frame).toContain("second message"); + }); + + it("truncates long user messages", () => { + const shortMessage = "Hello"; + const longMessage = + "This is a very long message that should be truncated because it exceeds the maximum length of 120 characters. We need to make sure it gets properly truncated with the right prefix and ellipsis."; + const items = [ + createUserMessage(shortMessage), + createUserMessage(longMessage), + ]; + + const { lastFrame } = render( + , + ); + const frame = lastFrame()!; + + // Short message should have the > prefix + expect(frame).toContain(`> ${shortMessage}`); + + // Long message should be truncated and contain: + // 1. The > prefix + expect(frame).toContain("> This is a very long message"); + // 2. An ellipsis indicating truncation + expect(frame).toContain("…"); + // 3. Not contain the full message + expect(frame).not.toContain(longMessage); + + // Find the truncated message line + const lines = frame.split("\n"); + const truncatedLine = lines.find((line) => + line.includes("This is a very long message"), + )!; + // Verify it's not too long (allowing for some UI elements) + expect(truncatedLine.trim().length).toBeLessThan(150); + }); + }); + + describe("file mode", () => { + it("displays files from shell commands", () => { + const items = [ + createFunctionCall("shell", { cmd: ["cat", "/path/to/file"] }), + ]; + const { lastFrame } = render( + , + ); + + // Switch to file mode + keyboardHandler?.("f", {}); + const frame = lastFrame(); + expect(frame).toContain("Files touched"); + expect(frame).toContain("/path/to/file"); + }); + + it("displays files from read operations", () => { + const items = [ + createFunctionCall("read_file", { path: "/path/to/file" }), + ]; + const { lastFrame } = render( + , + ); + + // Switch to file mode + keyboardHandler?.("f", {}); + const frame = lastFrame(); + expect(frame).toContain("Files touched"); + expect(frame).toContain("/path/to/file"); + }); + + it("displays files from patches", () => { + const items = [ + createFunctionCall("shell", { + cmd: [ + "apply_patch", + "*** Begin Patch\n--- a/src/file1.txt\n+++ b/src/file1.txt\n@@ -1,5 +1,5 @@\n-const x = 1;\n+const x = 2;\n", + ], + }), + ]; + const { lastFrame } = render( + , + ); + + // Switch to file mode + keyboardHandler?.("f", {}); + const frame = lastFrame(); + expect(frame).toContain("Files touched"); + expect(frame).toContain("src/file1.txt"); + }); + }); + + describe("keyboard interaction", () => { + it("handles mode switching with 'c' and 'f' keys", () => { + const items = [ + createUserMessage("hello"), + createFunctionCall("shell", { cmd: ["cat", "src/test.txt"] }), + ]; + const { lastFrame } = render( + , + ); + + // Initial state (command mode) + let frame = lastFrame(); + expect(frame).toContain("Commands run"); + expect(frame).toContain("hello"); + expect(frame).toContain("cat src/test.txt"); + + // Switch to files mode + keyboardHandler?.("f", {}); + frame = lastFrame(); + expect(frame).toContain("Files touched"); + expect(frame).toContain("src/test.txt"); + + // Switch back to commands mode + keyboardHandler?.("c", {}); + frame = lastFrame(); + expect(frame).toContain("Commands run"); + expect(frame).toContain("hello"); + expect(frame).toContain("cat src/test.txt"); + }); + + it("handles escape key", () => { + const onExit = vi.fn(); + render(); + + keyboardHandler?.("", { escape: true }); + expect(onExit).toHaveBeenCalled(); + }); + + it("handles arrow keys for navigation", () => { + const items = [createUserMessage("first"), createUserMessage("second")]; + const { lastFrame } = render( + , + ); + + // Initial state shows first item selected + let frame = lastFrame(); + expect(frame).toContain("› > first"); + expect(frame).not.toContain("› > second"); + + // Move down - second item should be selected + keyboardHandler?.("", { downArrow: true }); + frame = lastFrame(); + expect(frame).toContain("› > second"); + expect(frame).not.toContain("› > first"); + + // Move up - first item should be selected again + keyboardHandler?.("", { upArrow: true }); + frame = lastFrame(); + expect(frame).toContain("› > first"); + expect(frame).not.toContain("› > second"); + }); + + it("handles page up/down navigation", () => { + const items = Array.from({ length: 12 }, (_, i) => + createUserMessage(`message ${i + 1}`), + ); + + const { lastFrame } = render( + , + ); + + // Initial position - first message selected + let frame = lastFrame(); + expect(frame).toMatch(/│ › > message 1\s+│/); // message 1 should be selected + expect(frame).toMatch(/│ {3}> message 11\s+│/); // message 11 should be visible but not selected + + // Page down moves by 10 - message 11 should be selected + keyboardHandler?.("", { pageDown: true }); + frame = lastFrame(); + expect(frame).toMatch(/│ {3}> message 1\s+│/); // message 1 should be visible but not selected + expect(frame).toMatch(/│ › > message 11\s+│/); // message 11 should be selected + }); + + it("handles vim-style navigation", () => { + const items = [ + createUserMessage("first"), + createUserMessage("second"), + createUserMessage("third"), + ]; + const { lastFrame } = render( + , + ); + + // Initial state should show first item selected + let frame = lastFrame(); + expect(frame).toContain("› > first"); + expect(frame).not.toContain("› > third"); // Make sure third is not selected initially + + // Test G to jump to end - third should be selected + keyboardHandler?.("G", {}); + frame = lastFrame(); + expect(frame).toContain("› > third"); + + // Test g to jump to beginning - first should be selected again + keyboardHandler?.("g", {}); + frame = lastFrame(); + expect(frame).toContain("› > first"); + }); + }); + + describe("error handling", () => { + it("handles empty or invalid items", () => { + const items = [{ type: "invalid" } as any, null as any, undefined as any]; + const { lastFrame } = render( + , + ); + // Should render without errors + expect(lastFrame()).toBeTruthy(); + }); + }); +});