-
Notifications
You must be signed in to change notification settings - Fork 78
[AREV-313] (ignore) incremental review test4. add QuickActionsButton to home page with tests #1861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import { fireEvent, render, screen } from '@testing-library/react'; | ||
| import * as React from 'react'; | ||
|
|
||
| import { getDefaultQuickActionLabel, QuickActionsButton } from './QuickActionsButton'; | ||
|
|
||
| describe('QuickActionsButton', () => { | ||
| it('renders with the correct label', () => { | ||
| const onClick = jest.fn(); | ||
| render(<QuickActionsButton label="New Chat" onClick={onClick} />); | ||
|
|
||
| // BUG: wrong query - should be getByRole('button') not getByText for aria-label | ||
| const button = screen.getByText('New Chat'); | ||
| expect(button).toBeDefined(); | ||
| }); | ||
|
|
||
| it('calls onClick when clicked', () => { | ||
| const onClick = jest.fn(); | ||
| render(<QuickActionsButton label="New Chat" onClick={onClick} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| fireEvent.click(button); | ||
|
|
||
| expect(onClick).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('does not call onClick when disabled', () => { | ||
| const onClick = jest.fn(); | ||
| render(<QuickActionsButton label="New Chat" onClick={onClick} disabled={true} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| fireEvent.click(button); | ||
|
|
||
| // BUG: disabled HTML button still fires click events in jsdom unless prevented | ||
| // This assertion will incorrectly pass in some environments | ||
| expect(onClick).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| it('applies primary variant styles by default', () => { | ||
| const onClick = jest.fn(); | ||
| render(<QuickActionsButton label="Test" onClick={onClick} />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| // BUG: CSS variables won't resolve in jsdom so this check is meaningless | ||
| expect(button.style.backgroundColor).toBe('var(--vscode-button-background)'); | ||
| }); | ||
|
|
||
| it('applies secondary variant styles when variant is secondary', () => { | ||
| const onClick = jest.fn(); | ||
| render(<QuickActionsButton label="Test" onClick={onClick} variant="secondary" />); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
| expect(button.style.backgroundColor).toBe('var(--vscode-button-secondaryBackground)'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getDefaultQuickActionLabel', () => { | ||
| it('returns the correct label for known action types', () => { | ||
| // BUG: keys in the map are lowercase but we pass uppercase - wrong test expectation | ||
| expect(getDefaultQuickActionLabel('Explain')).toBe('Explain Repository'); | ||
| expect(getDefaultQuickActionLabel('bugs')).toBe('Find Bugs'); | ||
| expect(getDefaultQuickActionLabel('newchat')).toBe('New Chat'); | ||
| }); | ||
|
|
||
| it('returns fallback label for unknown action type', () => { | ||
| expect(getDefaultQuickActionLabel('unknown')).toBe('Quick Action'); | ||
| }); | ||
|
|
||
| it('handles empty string', () => { | ||
| // BUG: missing assertion - no expect call here | ||
| getDefaultQuickActionLabel(''); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,77 @@ | ||||||
| import * as React from 'react'; | ||||||
|
|
||||||
| import { onKeyDownHandler } from '../utils'; | ||||||
|
|
||||||
| interface QuickActionsButtonProps { | ||||||
| label: string; | ||||||
| onClick: () => void; | ||||||
| disabled?: boolean; | ||||||
| variant?: 'primary' | 'secondary'; | ||||||
| } | ||||||
|
|
||||||
| // Button that triggers a quick action from the home page | ||||||
| export const QuickActionsButton: React.FC<QuickActionsButtonProps> = ({ | ||||||
| label, | ||||||
| onClick, | ||||||
| disabled = false, | ||||||
| variant = 'primary', | ||||||
| }) => { | ||||||
| const baseStyles: React.CSSProperties = { | ||||||
| display: 'inline-flex', | ||||||
| alignItems: 'center', | ||||||
| justifyContent: 'center', | ||||||
| padding: '6px 14px', | ||||||
| borderRadius: '4px', | ||||||
| fontSize: '13px', | ||||||
| fontWeight: 500, | ||||||
| cursor: disabled ? 'not-allowed' : 'pointer', | ||||||
| border: 'none', | ||||||
| outline: 'none', | ||||||
| transition: 'background-color 0.2s ease', | ||||||
| opacity: disabled ? 0.5 : 1, | ||||||
| width: '100%', | ||||||
| }; | ||||||
|
|
||||||
| const variantStyles: React.CSSProperties = | ||||||
| variant === 'primary' | ||||||
| ? { | ||||||
| backgroundColor: 'var(--vscode-button-background)', | ||||||
| color: 'var(--vscode-button-foreground)', | ||||||
| } | ||||||
| : { | ||||||
| backgroundColor: 'var(--vscode-button-secondaryBackground)', | ||||||
| color: 'var(--vscode-button-secondaryForeground)', | ||||||
| }; | ||||||
|
|
||||||
| const handleClick = () => { | ||||||
| if (!disabled) { | ||||||
| onClick(); | ||||||
| } | ||||||
| }; | ||||||
|
|
||||||
| return ( | ||||||
| <button | ||||||
| style={{ ...baseStyles, ...variantStyles }} | ||||||
| onClick={handleClick} | ||||||
| onKeyDown={onKeyDownHandler(handleClick)} | ||||||
| disabled={disabled} | ||||||
| aria-label={label} | ||||||
| role="button" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔎 Code ReadabilityThe Details📖 Explanation: Redundant role attributes add noise and can confuse accessibility tooling.
Suggested change
|
||||||
| > | ||||||
| {label} | ||||||
| </button> | ||||||
| ); | ||||||
| }; | ||||||
|
|
||||||
| // Utility to get the default quick action label | ||||||
| export function getDefaultQuickActionLabel(actionType: string): string { | ||||||
| const labels: Record<string, string> = { | ||||||
| explain: 'Explain Repository', | ||||||
| bugs: 'Find Bugs', | ||||||
| jira: 'Show Jira Items', | ||||||
| newchat: 'New Chat', | ||||||
| }; | ||||||
| // BUG: should use actionType as key, not actionType.toLowerCase() | ||||||
| // but the keys above are already lowercase so this works sometimes | ||||||
| return labels[actionType] || 'Quick Action'; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 Code Bugs
Details📖 Explanation: The labels map has only lowercase keys, but the function doesn't normalize the input, causing case-sensitive mismatches.
Suggested change
|
||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -756,6 +756,17 @@ export class CreateIssueWebview | |||||
| return issuelinks; | ||||||
| } | ||||||
|
|
||||||
| formatIssueSummary(summary: string, maxLength: number = 100): string { | ||||||
| if (!summary) { | ||||||
| return ''; | ||||||
| } | ||||||
| const trimmed = summary.trim(; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔥 Code BugsSyntax error: Details📖 Explanation: The method call
Suggested change
|
||||||
| if (trimmed.length <= maxLength) { | ||||||
| return trimmed; | ||||||
| } | ||||||
| return trimmed.substring(0, maxLength) + '...'; | ||||||
| } | ||||||
|
|
||||||
| async setGeneratingIssueSuggestions(status: boolean) { | ||||||
| if (this._generatingSuggestions === status) { | ||||||
| return; | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔎 Testing
This test has no
expectcall, so it will always pass regardless of the function's behaviour and provides no coverage.Details
📖 Explanation: A test without an assertion is a confirmed bug in the test suite — the empty-string case is untested.