From 0ad6be7e6e5f1d6429bdfa365e1dadbf9504c815 Mon Sep 17 00:00:00 2001 From: Daniel <57051444+daniel-lxs@users.noreply.github.com> Date: Sun, 1 Jun 2025 22:14:20 -0500 Subject: [PATCH] Unify history item UI with TaskItem and TaskItemHeader (#4151) * refactor: Unify history item UI with TaskItem and TaskItemHeader Introduces `TaskItem.tsx` and `TaskItemHeader.tsx` to centralize and standardize the rendering of history entries. `TaskItem` handles the overall structure for "compact" (Preview) and "full" (HistoryView) variants. `TaskItemHeader` consolidates all metadata (timestamp, tokens, cost, cache, file size) into a single, consistent line above the task content, enhancing visual clarity and reducing UI clutter. This refactor significantly simplifies `HistoryPreview.tsx` and `HistoryView.tsx`. Approximately 314 lines of previous rendering logic were removed from these components and replaced by 242 lines in the new, focused, and reusable `TaskItem` and `TaskItemHeader` components, resulting in a net reduction and improved maintainability. Most importantly, rendering logic happens in one place. Key UI Changes: - Metadata (timestamp, tokens, cost, cache, file size) now displayed inline on a single header row in both variants. - Removed explicit "Tokens:" and "API Cost:" labels for a cleaner look. - Action buttons (Copy, Export, Delete) in the full view are now aligned with the metadata header. - File size is displayed in the header for the "full" variant only. - Workspace information is no longer displayed in the "compact" preview. Component Changes: - Created `webview-ui/src/components/history/TaskItem.tsx` (125 lines) - Created `webview-ui/src/components/history/TaskItemHeader.tsx` (117 lines) - Modified `webview-ui/src/components/history/HistoryPreview.tsx` (-65 lines, +3 lines) - Modified `webview-ui/src/components/history/HistoryView.tsx` (-249 lines, +3 lines) - Uses `HistoryItem` type for standardized data handling. Fixes: #4018 Signed-off-by: Eric Wheeler * test: fix TaskItem and HistoryView test failures after refactor Fixes test failures that occurred after the major refactoring that introduced the shared TaskItem component. The original implementation was correct but the tests needed updates to match the new component structure. - Add data-testid attributes to TaskItemHeader for reliable test selection - Update TaskItem.test.tsx assertions to use new test IDs for tokens/cache - Fix Checkbox import path in TaskItem.tsx (ui/checkbox vs ui) - Add missing mocks for lucide-react and Checkbox in HistoryView.test.tsx - Update HistoryView test assertions to use correct selectors - Ensure all 19 history component tests pass successfully The refactoring reduced code duplication by ~250+ lines while maintaining functionality, and these test fixes ensure the quality gates remain intact. Signed-off-by: Eric Wheeler * ui: move token and cost info from header to dedicated footer Create a new TaskItemFooter component that displays token and cost information with different styles for compact and full views. Move the CopyButton and ExportButton from header to footer in full view. Adjust file size display positioning in the header for better visual alignment. Signed-off-by: Eric Wheeler * test: update history component tests to match implementation Update data-testid attributes in HistoryView and TaskItem tests to match the actual implementation in TaskItemFooter component. The tests were looking for generic "tokens-in" and "tokens-out" attributes, but the implementation uses variant-specific attributes like "tokens-in-footer-full". Also added mocks for CopyButton and ExportButton components to resolve "Element type is invalid" errors during test rendering. Signed-off-by: Eric Wheeler * Move cache information from header to footer to be consistent with cost/token data * Fix ESLint warnings: remove unused imports and variables --------- Signed-off-by: Eric Wheeler Co-authored-by: Eric Wheeler --- .../src/components/history/HistoryPreview.tsx | 68 +---- .../src/components/history/HistoryView.tsx | 252 +----------------- .../src/components/history/TaskItem.tsx | 129 +++++++++ .../src/components/history/TaskItemFooter.tsx | 118 ++++++++ .../src/components/history/TaskItemHeader.tsx | 74 +++++ .../history/__tests__/HistoryView.test.tsx | 46 +++- .../history/__tests__/TaskItem.test.tsx | 117 ++++++++ 7 files changed, 496 insertions(+), 308 deletions(-) create mode 100644 webview-ui/src/components/history/TaskItem.tsx create mode 100644 webview-ui/src/components/history/TaskItemFooter.tsx create mode 100644 webview-ui/src/components/history/TaskItemHeader.tsx create mode 100644 webview-ui/src/components/history/__tests__/TaskItem.test.tsx diff --git a/webview-ui/src/components/history/HistoryPreview.tsx b/webview-ui/src/components/history/HistoryPreview.tsx index 080cbff3e2..892a8a220f 100644 --- a/webview-ui/src/components/history/HistoryPreview.tsx +++ b/webview-ui/src/components/history/HistoryPreview.tsx @@ -1,67 +1,21 @@ import { memo } from "react" -import { vscode } from "@/utils/vscode" -import { formatLargeNumber, formatDate } from "@/utils/format" - -import { CopyButton } from "./CopyButton" import { useTaskSearch } from "./useTaskSearch" - -import { Coins } from "lucide-react" +import TaskItem from "./TaskItem" const HistoryPreview = () => { - const { tasks, showAllWorkspaces } = useTaskSearch() + const { tasks } = useTaskSearch() return ( - <> -
- {tasks.length !== 0 && ( - <> - {tasks.slice(0, 3).map((item) => ( -
vscode.postMessage({ type: "showTaskWithId", text: item.id })}> -
-
- - {formatDate(item.ts)} - - -
-
- {item.task} -
-
- ↑ {formatLargeNumber(item.tokensIn || 0)} - ↓ {formatLargeNumber(item.tokensOut || 0)} - {!!item.totalCost && ( - - {" "} - {"$" + item.totalCost?.toFixed(2)} - - )} -
- {showAllWorkspaces && item.workspace && ( -
- - {item.workspace} -
- )} -
-
- ))} - - )} -
- +
+ {tasks.length !== 0 && ( + <> + {tasks.slice(0, 3).map((item) => ( + + ))} + + )} +
) } diff --git a/webview-ui/src/components/history/HistoryView.tsx b/webview-ui/src/components/history/HistoryView.tsx index bcb86a8d13..ab771e492f 100644 --- a/webview-ui/src/components/history/HistoryView.tsx +++ b/webview-ui/src/components/history/HistoryView.tsx @@ -1,21 +1,17 @@ import React, { memo, useState } from "react" import { DeleteTaskDialog } from "./DeleteTaskDialog" import { BatchDeleteTaskDialog } from "./BatchDeleteTaskDialog" -import prettyBytes from "pretty-bytes" import { Virtuoso } from "react-virtuoso" import { VSCodeTextField, VSCodeRadioGroup, VSCodeRadio } from "@vscode/webview-ui-toolkit/react" -import { vscode } from "@/utils/vscode" -import { formatLargeNumber, formatDate } from "@/utils/format" import { cn } from "@/lib/utils" import { Button, Checkbox } from "@/components/ui" import { useAppTranslation } from "@/i18n/TranslationContext" import { Tab, TabContent, TabHeader } from "../common/Tab" import { useTaskSearch } from "./useTaskSearch" -import { ExportButton } from "./ExportButton" -import { CopyButton } from "./CopyButton" +import TaskItem from "./TaskItem" type HistoryViewProps = { onDone: () => void @@ -210,245 +206,19 @@ const HistoryView = ({ onDone }: HistoryViewProps) => { )), }} itemContent={(index, item) => ( -
{ - if (isSelectionMode) { - toggleTaskSelection(item.id, !selectedTaskIds.includes(item.id)) - } else { - vscode.postMessage({ type: "showTaskWithId", text: item.id }) - } - }}> -
- {/* Show checkbox in selection mode */} - {isSelectionMode && ( -
{ - e.stopPropagation() - }}> - - toggleTaskSelection(item.id, checked === true) - } - variant="description" - /> -
- )} - -
-
- - {formatDate(item.ts)} - -
- {!isSelectionMode && ( - - )} -
-
-
-
-
-
- - {t("history:tokensLabel")} - - - - {formatLargeNumber(item.tokensIn || 0)} - - - - {formatLargeNumber(item.tokensOut || 0)} - -
- {!item.totalCost && !isSelectionMode && ( -
- - -
- )} -
- - {!!item.cacheWrites && ( -
- - {t("history:cacheLabel")} - - - - +{formatLargeNumber(item.cacheWrites || 0)} - - - - {formatLargeNumber(item.cacheReads || 0)} - -
- )} - - {!!item.totalCost && ( -
-
- - {t("history:apiCostLabel")} - - - ${item.totalCost?.toFixed(4)} - -
- {!isSelectionMode && ( -
- - -
- )} -
- )} - - {showAllWorkspaces && item.workspace && ( -
- - {item.workspace} -
- )} -
-
-
-
+ /> )} /> diff --git a/webview-ui/src/components/history/TaskItem.tsx b/webview-ui/src/components/history/TaskItem.tsx new file mode 100644 index 0000000000..635be2e351 --- /dev/null +++ b/webview-ui/src/components/history/TaskItem.tsx @@ -0,0 +1,129 @@ +import { memo } from "react" +import type { HistoryItem } from "@roo-code/types" + +import { vscode } from "@/utils/vscode" +import { cn } from "@/lib/utils" +import { Checkbox } from "@/components/ui/checkbox" +import { useAppTranslation } from "@/i18n/TranslationContext" + +import TaskItemHeader from "./TaskItemHeader" +import TaskItemFooter from "./TaskItemFooter" + +interface TaskItemProps { + item: HistoryItem + variant: "compact" | "full" + showWorkspace?: boolean + isSelectionMode?: boolean + isSelected?: boolean + onToggleSelection?: (taskId: string, isSelected: boolean) => void + onDelete?: (taskId: string) => void + className?: string +} + +const TaskItem = ({ + item, + variant, + showWorkspace = false, + isSelectionMode = false, + isSelected = false, + onToggleSelection, + onDelete, + className, +}: TaskItemProps) => { + const { t } = useAppTranslation() + + const handleClick = () => { + if (isSelectionMode && onToggleSelection) { + onToggleSelection(item.id, !isSelected) + } else { + vscode.postMessage({ type: "showTaskWithId", text: item.id }) + } + } + + const isCompact = variant === "compact" + + return ( +
+
+ {/* Selection checkbox - only in full variant */} + {!isCompact && isSelectionMode && ( +
{ + e.stopPropagation() + }}> + onToggleSelection?.(item.id, checked === true)} + variant="description" + /> +
+ )} + +
+ {/* Header with metadata */} + + + {/* Task content */} +
+ {isCompact ? item.task : undefined} +
+ + {/* Task Item Footer */} + + + {/* Workspace info */} + {showWorkspace && item.workspace && ( +
+ + {item.workspace} +
+ )} +
+
+
+ ) +} + +export default memo(TaskItem) diff --git a/webview-ui/src/components/history/TaskItemFooter.tsx b/webview-ui/src/components/history/TaskItemFooter.tsx new file mode 100644 index 0000000000..b3e6e56371 --- /dev/null +++ b/webview-ui/src/components/history/TaskItemFooter.tsx @@ -0,0 +1,118 @@ +import React from "react" +import type { HistoryItem } from "@roo-code/types" +import { Coins } from "lucide-react" +import { formatLargeNumber } from "@/utils/format" +import { cn } from "@/lib/utils" +import { useAppTranslation } from "@/i18n/TranslationContext" +import { CopyButton } from "./CopyButton" +import { ExportButton } from "./ExportButton" + +export interface TaskItemFooterProps { + item: HistoryItem + variant: "compact" | "full" + isSelectionMode?: boolean +} + +const TaskItemFooter: React.FC = ({ item, variant, isSelectionMode = false }) => { + const { t } = useAppTranslation() + const isCompact = variant === "compact" + + const metadataIconWithTextAdjustStyle: React.CSSProperties = { + fontSize: "12px", + color: "var(--vscode-descriptionForeground)", + verticalAlign: "middle", + marginBottom: "-2px", + fontWeight: "bold", + } + + return ( +
+ {isCompact ? ( + <> + {/* Compact Cache */} + {!!item.cacheWrites && ( + + + {formatLargeNumber(item.cacheWrites || 0)} + + {formatLargeNumber(item.cacheReads || 0)} + + )} + + {/* Compact Tokens */} + {(item.tokensIn || item.tokensOut) && ( + <> + + ↑ {formatLargeNumber(item.tokensIn || 0)} + + + ↓ {formatLargeNumber(item.tokensOut || 0)} + + + )} + {/* Compact Cost */} + {!!item.totalCost && ( + + + {"$" + item.totalCost.toFixed(2)} + + )} + + ) : ( + <> +
+ {/* Cache Info */} + {!!item.cacheWrites && ( +
+ {t("history:cacheLabel")} + + + {formatLargeNumber(item.cacheWrites || 0)} + + + + {formatLargeNumber(item.cacheReads || 0)} + +
+ )} + + {/* Full Tokens */} + {(item.tokensIn || item.tokensOut) && ( +
+ {t("history:tokensLabel")} + + + {formatLargeNumber(item.tokensIn || 0)} + + + + {formatLargeNumber(item.tokensOut || 0)} + +
+ )} + {/* Full Cost */} + {!!item.totalCost && ( +
+ {t("history:apiCostLabel")} + {"$" + item.totalCost.toFixed(4)} +
+ )} +
+ {/* Action Buttons for non-compact view */} + {!isSelectionMode && ( +
+ + +
+ )} + + )} +
+ ) +} + +export default TaskItemFooter diff --git a/webview-ui/src/components/history/TaskItemHeader.tsx b/webview-ui/src/components/history/TaskItemHeader.tsx new file mode 100644 index 0000000000..0837d7e0b8 --- /dev/null +++ b/webview-ui/src/components/history/TaskItemHeader.tsx @@ -0,0 +1,74 @@ +import React from "react" +import type { HistoryItem } from "@roo-code/types" +import prettyBytes from "pretty-bytes" +import { vscode } from "@/utils/vscode" +import { formatDate } from "@/utils/format" +import { Button } from "@/components/ui" +import { CopyButton } from "./CopyButton" + +export interface TaskItemHeaderProps { + item: HistoryItem + variant: "compact" | "full" + isSelectionMode: boolean + t: (key: string, options?: any) => string + onDelete?: (taskId: string) => void +} + +const TaskItemHeader: React.FC = ({ item, variant, isSelectionMode, t, onDelete }) => { + const isCompact = variant === "compact" + + // Standardized icon styles + const actionIconStyle: React.CSSProperties = { + fontSize: "16px", + color: "var(--vscode-descriptionForeground)", + verticalAlign: "middle", + } + + const handleDeleteClick = (e: React.MouseEvent) => { + e.stopPropagation() + if (e.shiftKey) { + vscode.postMessage({ type: "deleteTaskWithId", text: item.id }) + } else if (onDelete) { + onDelete(item.id) + } + } + + return ( +
+
+ + {formatDate(item.ts)} + +
+ + {/* Action Buttons */} + {!isSelectionMode && ( +
+ {isCompact ? ( + + ) : ( + <> + {onDelete && ( + + )} + {!isCompact && item.size && ( + + {prettyBytes(item.size)} + + )} + + )} +
+ )} +
+ ) +} + +export default TaskItemHeader diff --git a/webview-ui/src/components/history/__tests__/HistoryView.test.tsx b/webview-ui/src/components/history/__tests__/HistoryView.test.tsx index b4d9f22c48..9057b23067 100644 --- a/webview-ui/src/components/history/__tests__/HistoryView.test.tsx +++ b/webview-ui/src/components/history/__tests__/HistoryView.test.tsx @@ -8,6 +8,20 @@ import { vscode } from "@src/utils/vscode" jest.mock("@src/context/ExtensionStateContext") jest.mock("@src/utils/vscode") jest.mock("@src/i18n/TranslationContext") +jest.mock("@/components/ui/checkbox", () => ({ + Checkbox: jest.fn(({ checked, onCheckedChange, ...props }) => ( + onCheckedChange(e.target.checked)} + {...props} + /> + )), +})) +jest.mock("lucide-react", () => ({ + DollarSign: () => $, +})) jest.mock("react-virtuoso", () => ({ Virtuoso: ({ data, itemContent }: any) => (
@@ -259,8 +273,22 @@ describe("HistoryView", () => { // Find first task container and check date format const taskContainer = screen.getByTestId("virtuoso-item-1") - const dateElement = within(taskContainer).getByText((content) => { - return content.includes("FEBRUARY 16") && content.includes("12:00 AM") + // Date is directly in TaskItemHeader, which is a child of TaskItem (rendered by virtuoso) + const dateElement = within(taskContainer).getByText((content, element) => { + if (!element) { + return false + } + const parent = element.parentElement + if (!parent) { + return false + } + return ( + element.tagName.toLowerCase() === "span" && + parent.classList.contains("flex") && + parent.classList.contains("items-center") && + content.includes("FEBRUARY 16") && + content.includes("12:00 AM") + ) }) expect(dateElement).toBeInTheDocument() }) @@ -272,10 +300,9 @@ describe("HistoryView", () => { // Find first task container const taskContainer = screen.getByTestId("virtuoso-item-1") - // Find token counts within the task container - const tokensContainer = within(taskContainer).getByTestId("tokens-container") - expect(within(tokensContainer).getByTestId("tokens-in")).toHaveTextContent("100") - expect(within(tokensContainer).getByTestId("tokens-out")).toHaveTextContent("50") + // Find token counts within the task container (TaskItem -> TaskItemFooter) + expect(within(taskContainer).getByTestId("tokens-in-footer-full")).toHaveTextContent("100") + expect(within(taskContainer).getByTestId("tokens-out-footer-full")).toHaveTextContent("50") }) it("displays cache information when available", () => { @@ -285,10 +312,9 @@ describe("HistoryView", () => { // Find second task container const taskContainer = screen.getByTestId("virtuoso-item-2") - // Find cache info within the task container - const cacheContainer = within(taskContainer).getByTestId("cache-container") - expect(within(cacheContainer).getByTestId("cache-writes")).toHaveTextContent("+50") - expect(within(cacheContainer).getByTestId("cache-reads")).toHaveTextContent("25") + // Find cache info within the task container (TaskItem -> TaskItemHeader) + expect(within(taskContainer).getByTestId("cache-writes")).toHaveTextContent("50") // No plus sign in formatLargeNumber + expect(within(taskContainer).getByTestId("cache-reads")).toHaveTextContent("25") }) it("handles export functionality", () => { diff --git a/webview-ui/src/components/history/__tests__/TaskItem.test.tsx b/webview-ui/src/components/history/__tests__/TaskItem.test.tsx new file mode 100644 index 0000000000..f2f368350e --- /dev/null +++ b/webview-ui/src/components/history/__tests__/TaskItem.test.tsx @@ -0,0 +1,117 @@ +import { render, screen, fireEvent } from "@testing-library/react" +import type { HistoryItem } from "@roo-code/types" +import TaskItem from "../TaskItem" +import { vscode } from "@src/utils/vscode" + +jest.mock("@src/utils/vscode") +jest.mock("@src/i18n/TranslationContext") +jest.mock("lucide-react", () => ({ + DollarSign: () => $, + Coins: () => , // Mock for Coins icon used in TaskItemFooter compact +})) +jest.mock("../CopyButton", () => ({ + CopyButton: jest.fn(() => ), +})) +jest.mock("../ExportButton", () => ({ + ExportButton: jest.fn(() => ), +})) + +const mockTask: HistoryItem = { + number: 1, + id: "test-task-1", + task: "Test task content", + ts: new Date("2022-02-16T00:00:00").getTime(), + tokensIn: 100, + tokensOut: 50, + totalCost: 0.002, + workspace: "test-workspace", +} + +describe("TaskItem", () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it("renders compact variant correctly", () => { + render() + + expect(screen.getByText("Test task content")).toBeInTheDocument() + // Check for tokens display + expect(screen.getByTestId("tokens-in-footer-compact")).toHaveTextContent("100") + expect(screen.getByTestId("tokens-out-footer-compact")).toHaveTextContent("50") + expect(screen.getByTestId("cost-footer-compact")).toHaveTextContent("$0.00") // Cost + }) + + it("renders full variant correctly", () => { + render() + + expect(screen.getByTestId("task-item-test-task-1")).toBeInTheDocument() + expect(screen.getByTestId("task-content")).toBeInTheDocument() + expect(screen.getByTestId("tokens-in-footer-full")).toHaveTextContent("100") + expect(screen.getByTestId("tokens-out-footer-full")).toHaveTextContent("50") + }) + + it("shows workspace when showWorkspace is true", () => { + render() + + expect(screen.getByText("test-workspace")).toBeInTheDocument() + }) + + it("handles click events correctly", () => { + render() + + fireEvent.click(screen.getByText("Test task content")) + + expect(vscode.postMessage).toHaveBeenCalledWith({ + type: "showTaskWithId", + text: "test-task-1", + }) + }) + + it("handles selection mode correctly", () => { + const mockToggleSelection = jest.fn() + render( + , + ) + + const checkbox = screen.getByRole("checkbox") + expect(checkbox).toBeInTheDocument() + expect(checkbox).not.toBeChecked() + + fireEvent.click(screen.getByTestId("task-item-test-task-1")) + + expect(mockToggleSelection).toHaveBeenCalledWith("test-task-1", true) + expect(vscode.postMessage).not.toHaveBeenCalled() + }) + + it("shows delete button in full variant when not in selection mode", () => { + const mockOnDelete = jest.fn() + render() + + const deleteButton = screen.getByTestId("delete-task-button") + expect(deleteButton).toBeInTheDocument() + + fireEvent.click(deleteButton) + + expect(mockOnDelete).toHaveBeenCalledWith("test-task-1") + }) + + it("displays cache information when available", () => { + const taskWithCache: HistoryItem = { + ...mockTask, + cacheWrites: 25, + cacheReads: 10, + } + + render() + + expect(screen.getByTestId("cache-writes")).toHaveTextContent("25") + expect(screen.getByTestId("cache-reads")).toHaveTextContent("10") + }) +})