From 4b27264a21f4213106e37985e50c84ba687dc2be Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Sun, 15 Dec 2024 22:38:21 -0500 Subject: [PATCH 1/5] Revert "Merge pull request #125 from RooVetGit/search_replace_prompt_hotfix" This reverts commit 7d7a8563b7bd466e6a9183900e9707df9c07add8, reversing changes made to 868f4cddc9435d748ab090c6f6258495ea95667f. --- CHANGELOG.md | 4 ---- package-lock.json | 4 ++-- package.json | 2 +- src/core/diff/strategies/search-replace.ts | 1 - 4 files changed, 3 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd73b4f..44844ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,5 @@ # Roo Cline Changelog -## [2.2.9] - -- Fix a bug where Gemini was including line numbers in the search/replace content - ## [2.2.8] - More work on diff editing (better matching, indentation, logging) diff --git a/package-lock.json b/package-lock.json index 744ce85..733c114 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "roo-cline", - "version": "2.2.9", + "version": "2.2.8", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "roo-cline", - "version": "2.2.9", + "version": "2.2.8", "dependencies": { "@anthropic-ai/bedrock-sdk": "^0.10.2", "@anthropic-ai/sdk": "^0.26.0", diff --git a/package.json b/package.json index 7d9d6d3..62c46fa 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "displayName": "Roo Cline", "description": "A fork of Cline, an autonomous coding agent, with some added experimental configuration and automation features.", "publisher": "RooVeterinaryInc", - "version": "2.2.9", + "version": "2.2.8", "icon": "assets/icons/rocket.png", "galleryBanner": { "color": "#617A91", diff --git a/src/core/diff/strategies/search-replace.ts b/src/core/diff/strategies/search-replace.ts index f9de1fe..21309c6 100644 --- a/src/core/diff/strategies/search-replace.ts +++ b/src/core/diff/strategies/search-replace.ts @@ -62,7 +62,6 @@ The tool will maintain proper indentation and formatting while making changes. Only a single operation is allowed per tool use. The SEARCH section must exactly match existing content including whitespace and indentation. If you're not confident in the exact content to search for, use the read_file tool first to get the exact content. -IMPORTANT: The read_file tool returns the file content with line numbers prepended to each line. However, DO NOT include line numbers in the SEARCH and REPLACE sections of the diff content. Parameters: - path: (required) The path of the file to modify (relative to the current working directory ${cwd}) From e6c79eff80c8ff0d9f51f2dddfb8b7a7fd66e841 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Sun, 15 Dec 2024 22:38:49 -0500 Subject: [PATCH 2/5] Instead of trusting the prompt, just strip line numbers from diff contents --- CHANGELOG.md | 10 +- package-lock.json | 4 +- package.json | 2 +- .../__tests__/search-replace.test.ts | 147 ++++++++++++++++++ src/core/diff/strategies/search-replace.ts | 17 +- 5 files changed, 167 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44844ba..85023ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,17 +1,9 @@ # Roo Cline Changelog -## [2.2.8] - -- More work on diff editing (better matching, indentation, logging) - -## [2.2.7] +## [2.2.6 - 2.2.10] - More fixes to search/replace diffs -## [2.2.6] - -- Add a fuzzy match tolerance when applying diffs - ## [2.2.5] - Allow MCP servers to be enabled/disabled diff --git a/package-lock.json b/package-lock.json index 733c114..3806ccb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "roo-cline", - "version": "2.2.8", + "version": "2.2.10", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "roo-cline", - "version": "2.2.8", + "version": "2.2.10", "dependencies": { "@anthropic-ai/bedrock-sdk": "^0.10.2", "@anthropic-ai/sdk": "^0.26.0", diff --git a/package.json b/package.json index 62c46fa..f2151db 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "displayName": "Roo Cline", "description": "A fork of Cline, an autonomous coding agent, with some added experimental configuration and automation features.", "publisher": "RooVeterinaryInc", - "version": "2.2.8", + "version": "2.2.10", "icon": "assets/icons/rocket.png", "galleryBanner": { "color": "#617A91", diff --git a/src/core/diff/strategies/__tests__/search-replace.test.ts b/src/core/diff/strategies/__tests__/search-replace.test.ts index e0b348d..8e48680 100644 --- a/src/core/diff/strategies/__tests__/search-replace.test.ts +++ b/src/core/diff/strategies/__tests__/search-replace.test.ts @@ -564,6 +564,153 @@ this.init(); }); }) + describe('line number stripping', () => { + describe('line number stripping', () => { + let strategy: SearchReplaceDiffStrategy + + beforeEach(() => { + strategy = new SearchReplaceDiffStrategy() + }) + + it('should strip line numbers from both search and replace sections', () => { + const originalContent = 'function test() {\n return true;\n}\n' + const diffContent = `test.ts +<<<<<<< SEARCH +1 | function test() { +2 | return true; +3 | } +======= +1 | function test() { +2 | return false; +3 | } +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('function test() {\n return false;\n}\n') + } + }) + + it('should not strip when not all lines have numbers in either section', () => { + const originalContent = 'function test() {\n return true;\n}\n' + const diffContent = `test.ts +<<<<<<< SEARCH +1 | function test() { +2 | return true; +3 | } +======= +1 | function test() { + return false; +3 | } +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent) + expect(result.success).toBe(false) + }) + + it('should preserve content that naturally starts with pipe', () => { + const originalContent = '|header|another|\n|---|---|\n|data|more|\n' + const diffContent = `test.ts +<<<<<<< SEARCH +1 | |header|another| +2 | |---|---| +3 | |data|more| +======= +1 | |header|another| +2 | |---|---| +3 | |data|updated| +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('|header|another|\n|---|---|\n|data|updated|\n') + } + }) + + it('should preserve indentation when stripping line numbers', () => { + const originalContent = ' function test() {\n return true;\n }\n' + const diffContent = `test.ts +<<<<<<< SEARCH +1 | function test() { +2 | return true; +3 | } +======= +1 | function test() { +2 | return false; +3 | } +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe(' function test() {\n return false;\n }\n') + } + }) + + it('should handle different line numbers between sections', () => { + const originalContent = 'function test() {\n return true;\n}\n' + const diffContent = `test.ts +<<<<<<< SEARCH +10 | function test() { +11 | return true; +12 | } +======= +20 | function test() { +21 | return false; +22 | } +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('function test() {\n return false;\n}\n') + } + }) + + it('should not strip content that starts with pipe but no line number', () => { + const originalContent = '| Pipe\n|---|\n| Data\n' + const diffContent = `test.ts +<<<<<<< SEARCH +| Pipe +|---| +| Data +======= +| Pipe +|---| +| Updated +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('| Pipe\n|---|\n| Updated\n') + } + }) + + it('should handle mix of line-numbered and pipe-only content', () => { + const originalContent = '| Pipe\n|---|\n| Data\n' + const diffContent = `test.ts +<<<<<<< SEARCH +| Pipe +|---| +| Data +======= +1 | | Pipe +2 | |---| +3 | | NewData +>>>>>>> REPLACE` + + const result = strategy.applyDiff(originalContent, diffContent) + expect(result.success).toBe(true) + if (result.success) { + expect(result.content).toBe('1 | | Pipe\n2 | |---|\n3 | | NewData\n') + } + }) + }) + }); + describe('fuzzy matching', () => { let strategy: SearchReplaceDiffStrategy diff --git a/src/core/diff/strategies/search-replace.ts b/src/core/diff/strategies/search-replace.ts index 21309c6..a950b08 100644 --- a/src/core/diff/strategies/search-replace.ts +++ b/src/core/diff/strategies/search-replace.ts @@ -131,10 +131,25 @@ Your search/replace content here }; } - const [_, searchContent, replaceContent] = match; + let [_, searchContent, replaceContent] = match; // Detect line ending from original content const lineEnding = originalContent.includes('\r\n') ? '\r\n' : '\n'; + + // Strip line numbers from search and replace content if every line starts with a line number + const hasLineNumbers = (content: string) => { + const lines = content.split(/\r?\n/); + return lines.length > 0 && lines.every(line => /^\d+\s+\|(?!\|)/.test(line)); + }; + + if (hasLineNumbers(searchContent) && hasLineNumbers(replaceContent)) { + const stripLineNumbers = (content: string) => { + return content.replace(/^\d+\s+\|(?!\|)/gm, '') + }; + + searchContent = stripLineNumbers(searchContent); + replaceContent = stripLineNumbers(replaceContent); + } // Split content into lines, handling both \n and \r\n const searchLines = searchContent.split(/\r?\n/); From deb4df4bc30427ba07d8dbb452a6412a09cc594a Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Sat, 14 Dec 2024 23:05:12 -0500 Subject: [PATCH 3/5] Fix webview tests --- package.json | 2 +- webview-ui/package.json | 4 +- .../src/components/history/HistoryView.tsx | 140 +++-- .../history/__tests__/HistoryView.test.tsx | 504 +++++++----------- .../mcp/__tests__/McpToolRow.test.tsx | 62 ++- webview-ui/src/setupTests.ts | 21 +- 6 files changed, 334 insertions(+), 399 deletions(-) diff --git a/package.json b/package.json index f2151db..2b605d3 100644 --- a/package.json +++ b/package.json @@ -157,7 +157,7 @@ "package": "npm run build:webview && npm run check-types && npm run lint && node esbuild.js --production", "pretest": "npm run compile-tests && npm run compile && npm run lint", "start:webview": "cd webview-ui && npm run start", - "test": "jest", + "test": "jest && npm run test:webview", "test:webview": "cd webview-ui && npm run test", "prepare": "husky", "publish:marketplace": "vsce publish", diff --git a/webview-ui/package.json b/webview-ui/package.json index 4a74211..3d12cb9 100644 --- a/webview-ui/package.json +++ b/webview-ui/package.json @@ -30,7 +30,7 @@ "scripts": { "start": "react-scripts start", "build": "node ./scripts/build-react-no-split.js", - "test": "react-scripts test", + "test": "react-scripts test --watchAll=false", "eject": "react-scripts eject" }, "eslintConfig": { @@ -57,7 +57,7 @@ }, "jest": { "transformIgnorePatterns": [ - "/node_modules/(?!(rehype-highlight|react-remark|unist-util-visit|vfile|unified|bail|is-plain-obj|trough|vfile-message|unist-util-stringify-position|mdast-util-from-markdown|mdast-util-to-string|micromark|decode-named-character-reference|character-entities|markdown-table|zwitch|longest-streak|escape-string-regexp|unist-util-is|hast-util-to-text)/)" + "/node_modules/(?!(rehype-highlight|react-remark|unist-util-visit|vfile|unified|bail|is-plain-obj|trough|vfile-message|unist-util-stringify-position|mdast-util-from-markdown|mdast-util-to-string|micromark|decode-named-character-reference|character-entities|markdown-table|zwitch|longest-streak|escape-string-regexp|unist-util-is|hast-util-to-text|@vscode/webview-ui-toolkit|@microsoft/fast-react-wrapper|@microsoft/fast-element|@microsoft/fast-foundation|@microsoft/fast-web-utilities|exenv-es6)/)" ], "moduleNameMapper": { "\\.(css|less|scss|sass)$": "identity-obj-proxy" diff --git a/webview-ui/src/components/history/HistoryView.tsx b/webview-ui/src/components/history/HistoryView.tsx index bec38af..28ff461 100644 --- a/webview-ui/src/components/history/HistoryView.tsx +++ b/webview-ui/src/components/history/HistoryView.tsx @@ -2,7 +2,7 @@ import { VSCodeButton, VSCodeTextField, VSCodeRadioGroup, VSCodeRadio } from "@v import { useExtensionState } from "../../context/ExtensionStateContext" import { vscode } from "../../utils/vscode" import { Virtuoso } from "react-virtuoso" -import { memo, useMemo, useState, useEffect } from "react" +import React, { memo, useMemo, useState, useEffect } from "react" import Fuse, { FuseResult } from "fuse.js" import { formatLargeNumber } from "../../utils/format" @@ -82,30 +82,28 @@ const HistoryView = ({ onDone }: HistoryViewProps) => { const taskHistorySearchResults = useMemo(() => { let results = searchQuery ? highlight(fuse.search(searchQuery)) : presentableTasks - results.sort((a, b) => { + // First apply search if needed + const searchResults = searchQuery ? results : presentableTasks; + + // Then sort the results + return [...searchResults].sort((a, b) => { switch (sortOption) { case "oldest": - return a.ts - b.ts + return (a.ts || 0) - (b.ts || 0); case "mostExpensive": - return (b.totalCost || 0) - (a.totalCost || 0) + return (b.totalCost || 0) - (a.totalCost || 0); case "mostTokens": - return ( - (b.tokensIn || 0) + - (b.tokensOut || 0) + - (b.cacheWrites || 0) + - (b.cacheReads || 0) - - ((a.tokensIn || 0) + (a.tokensOut || 0) + (a.cacheWrites || 0) + (a.cacheReads || 0)) - ) + const aTokens = (a.tokensIn || 0) + (a.tokensOut || 0) + (a.cacheWrites || 0) + (a.cacheReads || 0); + const bTokens = (b.tokensIn || 0) + (b.tokensOut || 0) + (b.cacheWrites || 0) + (b.cacheReads || 0); + return bTokens - aTokens; case "mostRelevant": - // NOTE: you must never sort directly on object since it will cause members to be reordered - return searchQuery ? 0 : b.ts - a.ts // Keep fuse order if searching, otherwise sort by newest + // Keep fuse order if searching, otherwise sort by newest + return searchQuery ? 0 : (b.ts || 0) - (a.ts || 0); case "newest": default: - return b.ts - a.ts + return (b.ts || 0) - (a.ts || 0); } - }) - - return results + }); }, [presentableTasks, searchQuery, fuse, sortOption]) return ( @@ -227,9 +225,16 @@ const HistoryView = ({ onDone }: HistoryViewProps) => { overflowY: "scroll", }} data={taskHistorySearchResults} + data-testid="virtuoso-container" + components={{ + List: React.forwardRef((props, ref) => ( +
+ )) + }} itemContent={(index, item) => (
{ {formatDate(item.ts)}
- handleCopyTask(e, item.task)}> - - - { - e.stopPropagation() - handleDeleteHistoryItem(item.id) - }} - className="delete-button"> - - + +
{ />
{ Tokens: { {formatLargeNumber(item.tokensIn || 0)} { {!!item.cacheWrites && (
{ Cache: { +{formatLargeNumber(item.cacheWrites || 0)} { - const start = region[0] - const end = region[1] - const lastRegionNextIndex = end + 1 - - content += [ - inputText.substring(nextUnhighlightedRegionStartingIndex, start), - ``, - inputText.substring(start, lastRegionNextIndex), - "", - ].join("") - - nextUnhighlightedRegionStartingIndex = lastRegionNextIndex + + // Convert regions to a list of parts with their highlight status + const parts: { text: string; highlight: boolean }[] = [] + let lastIndex = 0 + + mergedRegions.forEach(([start, end]) => { + // Add non-highlighted text before this region + if (start > lastIndex) { + parts.push({ + text: inputText.substring(lastIndex, start), + highlight: false + }) + } + + // Add highlighted text + parts.push({ + text: inputText.substring(start, end + 1), + highlight: true + }) + + lastIndex = end + 1 }) - - content += inputText.substring(nextUnhighlightedRegionStartingIndex) - - return content + + // Add any remaining text + if (lastIndex < inputText.length) { + parts.push({ + text: inputText.substring(lastIndex), + highlight: false + }) + } + + // Build final string + return parts + .map(part => + part.highlight + ? `${part.text}` + : part.text + ) + .join('') } return fuseSearchResult diff --git a/webview-ui/src/components/history/__tests__/HistoryView.test.tsx b/webview-ui/src/components/history/__tests__/HistoryView.test.tsx index b38d2b3..3b7623e 100644 --- a/webview-ui/src/components/history/__tests__/HistoryView.test.tsx +++ b/webview-ui/src/components/history/__tests__/HistoryView.test.tsx @@ -1,362 +1,232 @@ import React from 'react' -import { render, screen, fireEvent, waitFor } from '@testing-library/react' +import { render, screen, fireEvent, within, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' import HistoryView from '../HistoryView' -import { ExtensionStateContextProvider } from '../../../context/ExtensionStateContext' +import { useExtensionState } from '../../../context/ExtensionStateContext' import { vscode } from '../../../utils/vscode' -import { highlight } from '../HistoryView' -import { FuseResult } from 'fuse.js' -// Mock vscode API -jest.mock('../../../utils/vscode', () => ({ - vscode: { - postMessage: jest.fn(), - }, +// Mock dependencies +jest.mock('../../../context/ExtensionStateContext') +jest.mock('../../../utils/vscode') +jest.mock('react-virtuoso', () => ({ + Virtuoso: ({ data, itemContent }: any) => ( +
+ {data.map((item: any, index: number) => ( +
+ {itemContent(index, item)} +
+ ))} +
+ ), })) -interface VSCodeButtonProps { - children: React.ReactNode; - onClick?: (e: any) => void; - appearance?: string; - className?: string; -} - -interface VSCodeTextFieldProps { - value?: string; - onInput?: (e: { target: { value: string } }) => void; - placeholder?: string; - style?: React.CSSProperties; -} - -interface VSCodeRadioGroupProps { - children?: React.ReactNode; - value?: string; - onChange?: (e: { target: { value: string } }) => void; - style?: React.CSSProperties; -} - -interface VSCodeRadioProps { - value: string; - children: React.ReactNode; - disabled?: boolean; - style?: React.CSSProperties; -} - -// Mock VSCode components -jest.mock('@vscode/webview-ui-toolkit/react', () => ({ - VSCodeButton: function MockVSCodeButton({ - children, - onClick, - appearance, - className - }: VSCodeButtonProps) { - return ( - - ) +const mockTaskHistory = [ + { + id: '1', + task: 'Test task 1', + ts: new Date('2022-02-16T00:00:00').getTime(), + tokensIn: 100, + tokensOut: 50, + totalCost: 0.002, }, - VSCodeTextField: function MockVSCodeTextField({ - value, - onInput, - placeholder, - style - }: VSCodeTextFieldProps) { - return ( - onInput?.({ target: { value: e.target.value } })} - placeholder={placeholder} - style={style} - /> - ) + { + id: '2', + task: 'Test task 2', + ts: new Date('2022-02-17T00:00:00').getTime(), + tokensIn: 200, + tokensOut: 100, + cacheWrites: 50, + cacheReads: 25, }, - VSCodeRadioGroup: function MockVSCodeRadioGroup({ - children, - value, - onChange, - style - }: VSCodeRadioGroupProps) { - return ( -
- {children} -
- ) - }, - VSCodeRadio: function MockVSCodeRadio({ - value, - children, - disabled, - style - }: VSCodeRadioProps) { - return ( - - ) - } -})) - -// Mock window.navigator.clipboard -Object.assign(navigator, { - clipboard: { - writeText: jest.fn(), - }, -}) - -// Mock window.postMessage to trigger state hydration -const mockPostMessage = (state: any) => { - window.postMessage({ - type: 'state', - state: { - version: '1.0.0', - taskHistory: [], - ...state - } - }, '*') -} +] describe('HistoryView', () => { - const mockOnDone = jest.fn() - const sampleHistory = [ - { - id: '1', - task: 'First task', - ts: Date.now() - 3000, - tokensIn: 100, - tokensOut: 50, - totalCost: 0.002 - }, - { - id: '2', - task: 'Second task', - ts: Date.now() - 2000, - tokensIn: 200, - tokensOut: 100, - totalCost: 0.004 - }, - { - id: '3', - task: 'Third task', - ts: Date.now() - 1000, - tokensIn: 300, - tokensOut: 150, - totalCost: 0.006 - } - ] - beforeEach(() => { + // Reset all mocks before each test jest.clearAllMocks() + jest.useFakeTimers() + + // Mock useExtensionState implementation + ;(useExtensionState as jest.Mock).mockReturnValue({ + taskHistory: mockTaskHistory, + }) }) - it('renders history items in correct order', () => { - render( - - - - ) - - mockPostMessage({ taskHistory: sampleHistory }) - - const historyItems = screen.getAllByText(/task/i) - expect(historyItems).toHaveLength(3) - expect(historyItems[0]).toHaveTextContent('Third task') - expect(historyItems[1]).toHaveTextContent('Second task') - expect(historyItems[2]).toHaveTextContent('First task') + afterEach(() => { + jest.useRealTimers() }) - it('handles sorting by different criteria', async () => { - render( - - - - ) + it('renders history items correctly', () => { + const onDone = jest.fn() + render() - mockPostMessage({ taskHistory: sampleHistory }) + // Check if both tasks are rendered + expect(screen.getByTestId('virtuoso-item-1')).toBeInTheDocument() + expect(screen.getByTestId('virtuoso-item-2')).toBeInTheDocument() + expect(screen.getByText('Test task 1')).toBeInTheDocument() + expect(screen.getByText('Test task 2')).toBeInTheDocument() + }) - // Test oldest sort - const oldestRadio = screen.getByTestId('radio-oldest') + it('handles search functionality', async () => { + const onDone = jest.fn() + render() + + // Get search input and radio group + const searchInput = screen.getByPlaceholderText('Fuzzy search history...') + const radioGroup = screen.getByRole('radiogroup') + + // Type in search + await userEvent.type(searchInput, 'task 1') + + // Check if sort option automatically changes to "Most Relevant" + const mostRelevantRadio = within(radioGroup).getByLabelText('Most Relevant') + expect(mostRelevantRadio).not.toBeDisabled() + + // Click and wait for radio update + fireEvent.click(mostRelevantRadio) + + // Wait for radio button to be checked + const updatedRadio = await within(radioGroup).findByRole('radio', { name: 'Most Relevant', checked: true }) + expect(updatedRadio).toBeInTheDocument() + }) + + it('handles sort options correctly', async () => { + const onDone = jest.fn() + render() + + const radioGroup = screen.getByRole('radiogroup') + + // Test changing sort options + const oldestRadio = within(radioGroup).getByLabelText('Oldest') fireEvent.click(oldestRadio) - let historyItems = screen.getAllByText(/task/i) - expect(historyItems[0]).toHaveTextContent('First task') - expect(historyItems[2]).toHaveTextContent('Third task') + // Wait for oldest radio to be checked + const checkedOldestRadio = await within(radioGroup).findByRole('radio', { name: 'Oldest', checked: true }) + expect(checkedOldestRadio).toBeInTheDocument() - // Test most expensive sort - const expensiveRadio = screen.getByTestId('radio-mostExpensive') - fireEvent.click(expensiveRadio) + const mostExpensiveRadio = within(radioGroup).getByLabelText('Most Expensive') + fireEvent.click(mostExpensiveRadio) - historyItems = screen.getAllByText(/task/i) - expect(historyItems[0]).toHaveTextContent('Third task') - expect(historyItems[2]).toHaveTextContent('First task') - - // Test most tokens sort - const tokensRadio = screen.getByTestId('radio-mostTokens') - fireEvent.click(tokensRadio) - - historyItems = screen.getAllByText(/task/i) - expect(historyItems[0]).toHaveTextContent('Third task') - expect(historyItems[2]).toHaveTextContent('First task') + // Wait for most expensive radio to be checked + const checkedExpensiveRadio = await within(radioGroup).findByRole('radio', { name: 'Most Expensive', checked: true }) + expect(checkedExpensiveRadio).toBeInTheDocument() }) - it('handles search functionality and auto-switches to most relevant sort', async () => { - render( - - - - ) + it('handles task selection', () => { + const onDone = jest.fn() + render() - mockPostMessage({ taskHistory: sampleHistory }) + // Click on first task + fireEvent.click(screen.getByText('Test task 1')) - const searchInput = screen.getByPlaceholderText('Fuzzy search history...') - fireEvent.change(searchInput, { target: { value: 'First' } }) - - const historyItems = screen.getAllByText(/task/i) - expect(historyItems).toHaveLength(1) - expect(historyItems[0]).toHaveTextContent('First task') - - // Verify sort switched to Most Relevant - const radioGroup = screen.getByRole('radiogroup') - expect(radioGroup.getAttribute('data-current-value')).toBe('mostRelevant') - - // Clear search and verify sort reverts - fireEvent.change(searchInput, { target: { value: '' } }) - expect(radioGroup.getAttribute('data-current-value')).toBe('newest') - }) - - it('handles copy functionality and shows/hides modal', async () => { - render( - - - - ) - - mockPostMessage({ taskHistory: sampleHistory }) - - const copyButtons = screen.getAllByRole('button', { hidden: true }) - .filter(button => button.className.includes('copy-button')) - - fireEvent.click(copyButtons[0]) - - expect(navigator.clipboard.writeText).toHaveBeenCalledWith('Third task') - - // Verify modal appears - await waitFor(() => { - expect(screen.getByText('Prompt Copied to Clipboard')).toBeInTheDocument() + // Verify vscode message was sent + expect(vscode.postMessage).toHaveBeenCalledWith({ + type: 'showTaskWithId', + text: '1', }) - - // Verify modal disappears - await waitFor(() => { - expect(screen.queryByText('Prompt Copied to Clipboard')).not.toBeInTheDocument() - }, { timeout: 2500 }) }) - it('handles delete functionality', () => { - render( - - - - ) + it('handles task deletion', () => { + const onDone = jest.fn() + render() - mockPostMessage({ taskHistory: sampleHistory }) - - const deleteButtons = screen.getAllByRole('button', { hidden: true }) - .filter(button => button.className.includes('delete-button')) + // Find and hover over first task + const taskContainer = screen.getByTestId('virtuoso-item-1') + fireEvent.mouseEnter(taskContainer) - fireEvent.click(deleteButtons[0]) + const deleteButton = within(taskContainer).getByTitle('Delete Task') + fireEvent.click(deleteButton) + // Verify vscode message was sent expect(vscode.postMessage).toHaveBeenCalledWith({ type: 'deleteTaskWithId', - text: '3' + text: '1', }) }) + it('handles task copying', async () => { + const mockClipboard = { + writeText: jest.fn().mockResolvedValue(undefined), + } + Object.assign(navigator, { clipboard: mockClipboard }) + + const onDone = jest.fn() + render() + + // Find and hover over first task + const taskContainer = screen.getByTestId('virtuoso-item-1') + fireEvent.mouseEnter(taskContainer) + + const copyButton = within(taskContainer).getByTitle('Copy Prompt') + await userEvent.click(copyButton) + + // Verify clipboard API was called + expect(navigator.clipboard.writeText).toHaveBeenCalledWith('Test task 1') + + // Wait for copy modal to appear + const copyModal = await screen.findByText('Prompt Copied to Clipboard') + expect(copyModal).toBeInTheDocument() + + // Fast-forward timers and wait for modal to disappear + jest.advanceTimersByTime(2000) + await waitFor(() => { + expect(screen.queryByText('Prompt Copied to Clipboard')).not.toBeInTheDocument() + }) + }) + + it('formats dates correctly', () => { + const onDone = jest.fn() + render() + + // 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') + }) + expect(dateElement).toBeInTheDocument() + }) + + it('displays token counts correctly', () => { + const onDone = jest.fn() + render() + + // 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') + }) + + it('displays cache information when available', () => { + const onDone = jest.fn() + render() + + // 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') + }) + it('handles export functionality', () => { - render( - - - - ) + const onDone = jest.fn() + render() - mockPostMessage({ taskHistory: sampleHistory }) - - const exportButtons = screen.getAllByRole('button', { hidden: true }) - .filter(button => button.className.includes('export-button')) + // Find and hover over second task + const taskContainer = screen.getByTestId('virtuoso-item-2') + fireEvent.mouseEnter(taskContainer) - fireEvent.click(exportButtons[0]) + const exportButton = within(taskContainer).getByText('EXPORT') + fireEvent.click(exportButton) + // Verify vscode message was sent expect(vscode.postMessage).toHaveBeenCalledWith({ type: 'exportTaskWithId', - text: '3' + text: '2', }) }) - - it('calls onDone when Done button is clicked', () => { - render( - - - - ) - - const doneButton = screen.getByText('Done') - fireEvent.click(doneButton) - - expect(mockOnDone).toHaveBeenCalled() - }) - - describe('highlight function', () => { - it('correctly highlights search matches', () => { - const testData = [{ - item: { text: 'Hello world' }, - matches: [{ key: 'text', value: 'Hello world', indices: [[0, 4]] }], - refIndex: 0 - }] as FuseResult[] - - const result = highlight(testData) - expect(result[0].text).toBe('Hello world') - }) - - it('handles multiple matches', () => { - const testData = [{ - item: { text: 'Hello world Hello' }, - matches: [{ - key: 'text', - value: 'Hello world Hello', - indices: [[0, 4], [11, 15]] - }], - refIndex: 0 - }] as FuseResult[] - - const result = highlight(testData) - expect(result[0].text).toBe( - 'Hello world ' + - 'Hello' - ) - }) - - it('handles overlapping matches', () => { - const testData = [{ - item: { text: 'Hello' }, - matches: [{ - key: 'text', - value: 'Hello', - indices: [[0, 2], [1, 4]] - }], - refIndex: 0 - }] as FuseResult[] - - const result = highlight(testData) - expect(result[0].text).toBe('Hello') - }) - }) -}) +}) \ No newline at end of file diff --git a/webview-ui/src/components/mcp/__tests__/McpToolRow.test.tsx b/webview-ui/src/components/mcp/__tests__/McpToolRow.test.tsx index ff708db..9f3cd96 100644 --- a/webview-ui/src/components/mcp/__tests__/McpToolRow.test.tsx +++ b/webview-ui/src/components/mcp/__tests__/McpToolRow.test.tsx @@ -9,6 +9,30 @@ jest.mock('../../../utils/vscode', () => ({ } })) +jest.mock('@vscode/webview-ui-toolkit/react', () => ({ + VSCodeCheckbox: function MockVSCodeCheckbox({ + children, + checked, + onChange + }: { + children?: React.ReactNode; + checked?: boolean; + onChange?: (e: React.ChangeEvent) => void; + }) { + return ( + + ) + } +})) + describe('McpToolRow', () => { const mockTool = { name: 'test-tool', @@ -33,18 +57,18 @@ describe('McpToolRow', () => { expect(screen.queryByText('Always allow')).not.toBeInTheDocument() }) - it('shows always allow checkbox when serverName is provided', () => { - render() + it('shows always allow checkbox when serverName and alwaysAllowMcp are provided', () => { + render() expect(screen.getByText('Always allow')).toBeInTheDocument() }) - + it('sends message to toggle always allow when checkbox is clicked', () => { - render() + render() const checkbox = screen.getByRole('checkbox') fireEvent.click(checkbox) - + expect(vscode.postMessage).toHaveBeenCalledWith({ type: 'toggleToolAlwaysAllow', serverName: 'test-server', @@ -52,29 +76,31 @@ describe('McpToolRow', () => { alwaysAllow: true }) }) - + it('reflects always allow state in checkbox', () => { const alwaysAllowedTool = { ...mockTool, alwaysAllow: true } - - render() + + render() - const checkbox = screen.getByRole('checkbox') - expect(checkbox).toBeChecked() + const checkbox = screen.getByRole('checkbox') as HTMLInputElement + expect(checkbox.checked).toBe(true) }) - + it('prevents event propagation when clicking the checkbox', () => { - const mockStopPropagation = jest.fn() - render() + const mockOnClick = jest.fn() + render( +
+ +
+ ) const container = screen.getByTestId('tool-row-container') - fireEvent.click(container, { - stopPropagation: mockStopPropagation - }) - - expect(mockStopPropagation).toHaveBeenCalled() + fireEvent.click(container) + + expect(mockOnClick).not.toHaveBeenCalled() }) it('displays input schema parameters when provided', () => { diff --git a/webview-ui/src/setupTests.ts b/webview-ui/src/setupTests.ts index 6a0fd12..dbba64a 100644 --- a/webview-ui/src/setupTests.ts +++ b/webview-ui/src/setupTests.ts @@ -1,5 +1,16 @@ -// jest-dom adds custom jest matchers for asserting on DOM nodes. -// allows you to do things like: -// expect(element).toHaveTextContent(/react/i) -// learn more: https://github.com/testing-library/jest-dom -import "@testing-library/jest-dom" +import '@testing-library/jest-dom'; + +// Mock window.matchMedia +Object.defineProperty(window, 'matchMedia', { + writable: true, + value: jest.fn().mockImplementation(query => ({ + matches: false, + media: query, + onchange: null, + addListener: jest.fn(), // Deprecated + removeListener: jest.fn(), // Deprecated + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + dispatchEvent: jest.fn(), + })), +}); From afc22f0d6e420b69c6102c6c48025d9845e1ffc1 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Sun, 15 Dec 2024 23:57:21 -0500 Subject: [PATCH 4/5] Hit enter to add auto-approved commands --- webview-ui/src/components/settings/SettingsView.tsx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index deab3d6..ffa45d0 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -245,6 +245,12 @@ const SettingsView = ({ onDone }: SettingsViewProps) => { setCommandInput(e.target.value)} + onKeyDown={(e: any) => { + if (e.key === 'Enter') { + e.preventDefault() + handleAddCommand() + } + }} placeholder="Enter command prefix (e.g., 'git ')" style={{ flexGrow: 1 }} /> From e14ccf55dbfa6651c312118aa7e39e52fea90ab2 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Sun, 15 Dec 2024 23:58:17 -0500 Subject: [PATCH 5/5] Update unit test command to run webview tests too --- .github/workflows/code-qa.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/code-qa.yml b/.github/workflows/code-qa.yml index 0a5f3c1..de3f054 100644 --- a/.github/workflows/code-qa.yml +++ b/.github/workflows/code-qa.yml @@ -42,4 +42,4 @@ jobs: run: npm run install:all - name: Run unit tests - run: npx jest \ No newline at end of file + run: npm test \ No newline at end of file