Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion webview-ui/src/components/history/HistoryPreview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const HistoryPreview = () => {
<div className="flex flex-col gap-3">
{tasks.length !== 0 && (
<>
{tasks.slice(0, 3).map((item) => (
{tasks.slice(0, 5).map((item) => (
<TaskItem key={item.id} item={item} variant="compact" />
))}
<button
Expand Down
14 changes: 8 additions & 6 deletions webview-ui/src/components/history/TaskItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const TaskItem = ({
key={item.id}
data-testid={`task-item-${item.id}`}
className={cn(
"cursor-pointer group bg-vscode-editor-background rounded relative overflow-hidden hover:border-vscode-toolbar-hoverBackground/60",
"cursor-pointer group bg-vscode-editor-background rounded relative overflow-hidden border border-transparent hover:bg-vscode-list-hoverBackground transition-colors",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hover effect looks good! Could we verify that the contrast ratio between text and meets WCAG accessibility standards? This would ensure the UI remains accessible for users with visual impairments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, thanks for the consideration.

className,
)}
onClick={handleClick}>
Expand All @@ -72,12 +72,14 @@ const TaskItem = ({
{/* Header with metadata */}
<TaskItemHeader item={item} isSelectionMode={isSelectionMode} onDelete={onDelete} />

{/* Task content */}
{/* Task content - always show up to 3 lines */}
<div
className={cn("overflow-hidden whitespace-pre-wrap text-vscode-foreground text-ellipsis", {
"text-base line-clamp-3": !isCompact,
"line-clamp-2": isCompact,
})}
className={cn(
"overflow-hidden whitespace-pre-wrap text-vscode-foreground text-ellipsis line-clamp-3",
{
"text-base": !isCompact,
},
)}
data-testid="task-content"
{...(item.highlight ? { dangerouslySetInnerHTML: { __html: item.highlight } } : {})}>
{item.highlight ? undefined : item.task}
Expand Down
38 changes: 7 additions & 31 deletions webview-ui/src/components/history/TaskItemFooter.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import React from "react"
import type { HistoryItem } from "@roo-code/types"
import { Coins, FileIcon } from "lucide-react"
import prettyBytes from "pretty-bytes"
import { formatLargeNumber } from "@/utils/format"
import { formatTimeAgo } from "@/utils/format"
import { CopyButton } from "./CopyButton"
import { ExportButton } from "./ExportButton"

Expand All @@ -15,36 +13,14 @@ export interface TaskItemFooterProps {
const TaskItemFooter: React.FC<TaskItemFooterProps> = ({ item, variant, isSelectionMode = false }) => {
return (
<div className="text-xs text-vscode-descriptionForeground flex justify-between items-center mt-1">
<div className="flex gap-2">
{!!(item.cacheReads || item.cacheWrites) && (
<span className="flex items-center" data-testid="cache-compact">
<i className="mr-1 codicon codicon-cloud-upload text-sm! text-vscode-descriptionForeground" />
<span className="inline-block mr-1">{formatLargeNumber(item.cacheWrites || 0)}</span>
<i className="mr-1 codicon codicon-cloud-download text-sm! text-vscode-descriptionForeground" />
<span>{formatLargeNumber(item.cacheReads || 0)}</span>
</span>
)}

{/* Full Tokens */}
{!!(item.tokensIn || item.tokensOut) && (
<span className="flex items-center gap-1">
<span data-testid="tokens-in-footer-compact">↑ {formatLargeNumber(item.tokensIn || 0)}</span>
<span data-testid="tokens-out-footer-compact">↓ {formatLargeNumber(item.tokensOut || 0)}</span>
</span>
)}
<div className="flex gap-3 items-center">
{/* Datetime with time-ago format */}
<span className="text-vscode-descriptionForeground">{formatTimeAgo(item.ts)}</span>

{/* Full Cost */}
{/* Cost */}
{!!item.totalCost && (
<span className="flex items-center">
<Coins className="inline-block size-[1em] mr-1" />
<span data-testid="cost-footer-compact">{"$" + item.totalCost.toFixed(2)}</span>
</span>
)}

{!!item.size && (
<span className="flex items-center">
<FileIcon className="inline-block size-[1em] mr-1" />
<span data-testid="size-footer-compact">{prettyBytes(item.size)}</span>
<span className="flex items-center" data-testid="cost-footer-compact">
{"$" + item.totalCost.toFixed(2)}
</span>
)}
</div>
Expand Down
32 changes: 11 additions & 21 deletions webview-ui/src/components/history/TaskItemHeader.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import React from "react"
import type { HistoryItem } from "@roo-code/types"
import { formatDate } from "@/utils/format"
import { DeleteButton } from "./DeleteButton"
import { cn } from "@/lib/utils"

export interface TaskItemHeaderProps {
item: HistoryItem
Expand All @@ -11,27 +9,19 @@ export interface TaskItemHeaderProps {
}

const TaskItemHeader: React.FC<TaskItemHeaderProps> = ({ item, isSelectionMode, onDelete }) => {
return (
<div
className={cn("flex justify-between items-center", {
// this is to balance out the margin when we don't have a delete button
// because the delete button sorta pushes the date up due to its size
"mb-1": !onDelete,
})}>
<div className="flex items-center flex-wrap gap-x-2 text-xs">
<span className="text-vscode-descriptionForeground font-medium text-sm uppercase">
{formatDate(item.ts)}
</span>
</div>

{/* Action Buttons */}
{!isSelectionMode && (
// Only show delete button if needed
if (!isSelectionMode && onDelete) {
return (
<div className="flex justify-end">
<div className="flex flex-row gap-0 items-center opacity-20 group-hover:opacity-50 hover:opacity-100">
{onDelete && <DeleteButton itemId={item.id} onDelete={onDelete} />}
<DeleteButton itemId={item.id} onDelete={onDelete} />
</div>
)}
</div>
)
</div>
)
}

// Return null if no header content is needed
return null
}

export default TaskItemHeader
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ describe("HistoryPreview", () => {
expect(screen.queryByTestId(/task-item-/)).not.toBeInTheDocument()
})

it("renders up to 3 tasks when tasks are available", () => {
it("renders up to 5 tasks when tasks are available", () => {
mockUseTaskSearch.mockReturnValue({
tasks: mockTasks,
searchQuery: "",
Expand All @@ -119,19 +119,19 @@ describe("HistoryPreview", () => {

render(<HistoryPreview />)

// Should render only the first 3 tasks
// Should render only the first 5 tasks
expect(screen.getByTestId("task-item-task-1")).toBeInTheDocument()
expect(screen.getByTestId("task-item-task-2")).toBeInTheDocument()
expect(screen.getByTestId("task-item-task-3")).toBeInTheDocument()
expect(screen.queryByTestId("task-item-task-4")).not.toBeInTheDocument()
expect(screen.queryByTestId("task-item-task-5")).not.toBeInTheDocument()
expect(screen.getByTestId("task-item-task-4")).toBeInTheDocument()
expect(screen.getByTestId("task-item-task-5")).toBeInTheDocument()
expect(screen.queryByTestId("task-item-task-6")).not.toBeInTheDocument()
})

it("renders all tasks when there are 3 or fewer", () => {
const threeTasks = mockTasks.slice(0, 3)
it("renders all tasks when there are 5 or fewer", () => {
const fiveTasks = mockTasks.slice(0, 5)
mockUseTaskSearch.mockReturnValue({
tasks: threeTasks,
tasks: fiveTasks,
searchQuery: "",
setSearchQuery: vi.fn(),
sortOption: "newest",
Expand All @@ -147,7 +147,9 @@ describe("HistoryPreview", () => {
expect(screen.getByTestId("task-item-task-1")).toBeInTheDocument()
expect(screen.getByTestId("task-item-task-2")).toBeInTheDocument()
expect(screen.getByTestId("task-item-task-3")).toBeInTheDocument()
expect(screen.queryByTestId("task-item-task-4")).not.toBeInTheDocument()
expect(screen.getByTestId("task-item-task-4")).toBeInTheDocument()
expect(screen.getByTestId("task-item-task-5")).toBeInTheDocument()
expect(screen.queryByTestId("task-item-task-6")).not.toBeInTheDocument()
})

it("renders only 1 task when there is only 1 task", () => {
Expand All @@ -172,7 +174,7 @@ describe("HistoryPreview", () => {

it("passes correct props to TaskItem components", () => {
mockUseTaskSearch.mockReturnValue({
tasks: mockTasks.slice(0, 3),
tasks: mockTasks.slice(0, 5),
searchQuery: "",
setSearchQuery: vi.fn(),
sortOption: "newest",
Expand All @@ -185,7 +187,7 @@ describe("HistoryPreview", () => {

render(<HistoryPreview />)

// Verify TaskItem was called with correct props for first 3 tasks
// Verify TaskItem was called with correct props for first 5 tasks
expect(mockTaskItem).toHaveBeenCalledWith(
expect.objectContaining({
item: mockTasks[0],
Expand All @@ -207,6 +209,20 @@ describe("HistoryPreview", () => {
}),
expect.anything(),
)
expect(mockTaskItem).toHaveBeenCalledWith(
expect.objectContaining({
item: mockTasks[3],
variant: "compact",
}),
expect.anything(),
)
expect(mockTaskItem).toHaveBeenCalledWith(
expect.objectContaining({
item: mockTasks[4],
variant: "compact",
}),
expect.anything(),
)
})

it("renders with correct container classes", () => {
Expand Down
30 changes: 8 additions & 22 deletions webview-ui/src/components/history/__tests__/TaskItem.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,47 +74,33 @@ describe("TaskItem", () => {
expect(screen.getByTestId("export")).toBeInTheDocument()
})

it("displays cache information when present", () => {
const mockTaskWithCache = {
...mockTask,
cacheReads: 10,
cacheWrites: 5,
}

it("displays time ago information", () => {
render(
<TaskItem
item={mockTaskWithCache}
item={mockTask}
variant="full"
isSelected={false}
onToggleSelection={vi.fn()}
isSelectionMode={false}
/>,
)

// Should display cache information in the footer
expect(screen.getByTestId("cache-compact")).toBeInTheDocument()
expect(screen.getByText("5")).toBeInTheDocument() // cache writes
expect(screen.getByText("10")).toBeInTheDocument() // cache reads
// Should display time ago format
expect(screen.getByText(/ago/)).toBeInTheDocument()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be helpful to add more specific test cases for edge cases? For example:

  • Very recent times ("less than a minute ago")
  • Times from different periods (hours, days, months ago)
  • Future timestamps (edge case handling)

This would ensure the time formatting works correctly across all scenarios.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be testing the library, I don't think it's necessary.

})

it("does not display cache information when not present", () => {
const mockTaskWithoutCache = {
...mockTask,
cacheReads: 0,
cacheWrites: 0,
}

it("applies hover effect class", () => {
render(
<TaskItem
item={mockTaskWithoutCache}
item={mockTask}
variant="full"
isSelected={false}
onToggleSelection={vi.fn()}
isSelectionMode={false}
/>,
)

// Cache section should not be present
expect(screen.queryByTestId("cache-compact")).not.toBeInTheDocument()
const taskItem = screen.getByTestId("task-item-1")
expect(taskItem).toHaveClass("hover:bg-vscode-list-hoverBackground")
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,11 @@ const mockItem = {
}

describe("TaskItemFooter", () => {
it("renders token information", () => {
it("renders time ago information", () => {
render(<TaskItemFooter item={mockItem} variant="full" />)

// Check for token counts using testids since the text is split across elements
expect(screen.getByTestId("tokens-in-footer-compact")).toBeInTheDocument()
expect(screen.getByTestId("tokens-out-footer-compact")).toBeInTheDocument()
// Should show time ago format
expect(screen.getByText(/ago/)).toBeInTheDocument()
})

it("renders cost information", () => {
Expand All @@ -43,31 +42,19 @@ describe("TaskItemFooter", () => {
expect(screen.getByTestId("export")).toBeInTheDocument()
})

it("renders cache information when present", () => {
const mockItemWithCache = {
...mockItem,
cacheReads: 5,
cacheWrites: 3,
}
it("hides export button in compact variant", () => {
render(<TaskItemFooter item={mockItem} variant="compact" />)

render(<TaskItemFooter item={mockItemWithCache} variant="full" />)

// Check for cache display using testid
expect(screen.getByTestId("cache-compact")).toBeInTheDocument()
expect(screen.getByText("3")).toBeInTheDocument() // cache writes
expect(screen.getByText("5")).toBeInTheDocument() // cache reads
// Should show copy button but not export button
expect(screen.getByTestId("copy-prompt-button")).toBeInTheDocument()
expect(screen.queryByTestId("export")).not.toBeInTheDocument()
})

it("does not render cache information when not present", () => {
const mockItemWithoutCache = {
...mockItem,
cacheReads: 0,
cacheWrites: 0,
}

render(<TaskItemFooter item={mockItemWithoutCache} variant="full" />)
it("hides action buttons in selection mode", () => {
render(<TaskItemFooter item={mockItem} variant="full" isSelectionMode={true} />)

// Cache section should not be present
expect(screen.queryByTestId("cache-compact")).not.toBeInTheDocument()
// Should not show any action buttons
expect(screen.queryByTestId("copy-prompt-button")).not.toBeInTheDocument()
expect(screen.queryByTestId("export")).not.toBeInTheDocument()
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,21 @@ const mockItem = {
}

describe("TaskItemHeader", () => {
it("renders date information", () => {
it("shows delete button when not in selection mode and onDelete is provided", () => {
render(<TaskItemHeader item={mockItem} isSelectionMode={false} onDelete={vi.fn()} />)

// TaskItemHeader shows the formatted date, not the task text
expect(screen.getByText(/\w+ \d{1,2}, \d{1,2}:\d{2} \w{2}/)).toBeInTheDocument() // Date format like "JUNE 14, 10:15 AM"
expect(screen.getByTestId("delete-task-button")).toBeInTheDocument()
})

it("shows delete button when not in selection mode", () => {
render(<TaskItemHeader item={mockItem} isSelectionMode={false} onDelete={vi.fn()} />)
it("does not show delete button in selection mode", () => {
render(<TaskItemHeader item={mockItem} isSelectionMode={true} onDelete={vi.fn()} />)

expect(screen.queryByTestId("delete-task-button")).not.toBeInTheDocument()
})

it("does not show delete button when onDelete is not provided", () => {
render(<TaskItemHeader item={mockItem} isSelectionMode={false} />)

expect(screen.getByRole("button")).toBeInTheDocument()
expect(screen.queryByTestId("delete-task-button")).not.toBeInTheDocument()
})
})
5 changes: 5 additions & 0 deletions webview-ui/src/utils/format.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import i18next from "i18next"
import { formatDistanceToNow } from "date-fns"

export function formatLargeNumber(num: number): string {
if (num >= 1e9) {
Expand Down Expand Up @@ -33,3 +34,7 @@ export const formatDate = (timestamp: number) => {

return dateStr.toUpperCase()
}

export const formatTimeAgo = (timestamp: number) => {
return formatDistanceToNow(new Date(timestamp), { addSuffix: true })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional? The function doesn't use locale configuration like does above. This means the time-ago text will always display in English regardless of the user's language setting. Consider passing locale configuration to :

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope for this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d love to avoid English showing up for all languages. Sounds like the library supports passing in the current locale?

}
Loading