valknar/llmx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(history-overlay): split into modular functions & add tests (fixes #402) (#403)

Browse Source 
## 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
This commit is contained in:
Jordan Docherty
2025-04-20 22:27:06 -07:00
committed by GitHub
parent f97557f59f
commit f6b12aa994
2 changed files with 447 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

176
codex-cli/src/components/history-overlay.tsx
View File

@@ -14,7 +14,10 @@ export default function HistoryOverlay({ items, onExit }: Props): JSX.Element {
const [mode, setMode] = useState<Mode>("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<ResponseItem>): {
function formatHistoryForDisplay(items: Array<ResponseItem>): {
commands: Array<string>;
files: Array<string>;
} {
@@ -103,33 +106,9 @@ function buildLists(items: Array<ResponseItem>): {
const filesSet = new Set<string>();
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<unknown> }).content ?? [];
const texts: Array<string> = [];
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<ResponseItem>): {
: 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<ResponseItem>): {
// 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<string, unknown>)[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<unknown> }).content ?? [];
const texts: Array<string> = [];
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<string>,
filesSet: Set<string>,
): 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>,
): 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<string, unknown>)[key];
if (typeof val === "string") {
summary += ` ${val}`;
if (val.includes("/")) {
filesSet.add(val);
}
break;
}
}
}
return summary;
}

350
codex-cli/tests/history-overlay.test.tsx Normal file
View File

@@ -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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
// 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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
// 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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
// 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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
// 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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
// 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(<HistoryOverlay items={[]} onExit={onExit} />);
keyboardHandler?.("", { escape: true });
expect(onExit).toHaveBeenCalled();
});
it("handles arrow keys for navigation", () => {
const items = [createUserMessage("first"), createUserMessage("second")];
const { lastFrame } = render(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
// 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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
// 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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
// 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(
<HistoryOverlay items={items} onExit={vi.fn()} />,
);
// Should render without errors
expect(lastFrame()).toBeTruthy();
});
});
});