-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add mode display indicators on task cards (#6493) #6498
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
feat: add mode display indicators on task cards (#6493) #6498
Conversation
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.
Thank you for implementing the mode display indicators! This is a well-structured implementation that addresses the issue requirements. I've identified a couple of critical issues that need to be fixed before this can be merged, along with some suggestions for improvement.
|
|
||
| // Get all modes (built-in + custom) | ||
| const allModes = getAllModes(customModes) | ||
| const mode = findModeBySlug(modeSlug, allModes) |
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.
Should this be using getModeBySlug() instead of findModeBySlug()? Looking at the modes file, getModeBySlug() checks custom modes first then built-in modes, which seems more appropriate for this use case. findModeBySlug() only searches within the provided modes array.
| <StandardTooltip content={displayName}> | ||
| <span | ||
| className={cn( | ||
| "inline-flex items-center px-2 py-0.5 rounded text-xs font-medium", |
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.
Is the items-center class necessary when using truncate? The truncate behavior might not need vertical centering. Could we simplify this to just the essential classes?
| })) | ||
|
|
||
| // Mock the ExtensionStateContext | ||
| const { mockGetCurrentTaskItem, mockSetCurrentTaskItem } = vi.hoisted(() => { |
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.
The mock setup here seems quite complex for the test needs. Could we simplify this mock to improve maintainability? The hoisted pattern is good, but the getter/setter functions might be overkill for these tests.
| expect(badge).toHaveClass("max-w-[120px]") | ||
| }) | ||
|
|
||
| it("shows full name in tooltip", async () => { |
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.
This test has a placeholder comment but doesn't actually verify tooltip behavior. Could we either implement proper tooltip testing (with user interactions) or remove this incomplete test to avoid confusion?
- Changed from findModeBySlug to getModeBySlug for better mode resolution - Removed unnecessary 'items-center' CSS class from ModeBadge - Simplified mock setup in TaskHeader.spec.tsx for better maintainability - Improved tooltip test clarity in ModeBadge.spec.tsx - All tests passing successfully
Add mode display indicators on task cards
Fixes #6493
Summary
This PR adds mode display indicators to task cards in both the main chat view and history view, allowing users to easily see which mode was used for each task. The implementation includes a new reusable
ModeBadgecomponent that displays mode names with proper styling and handles edge cases gracefully.Changes Made
New Component
ModeBadge.tsx: Created a reusable component that:findModeBySlugfunctionUpdated Components
TaskItemHeader.tsx(History View):TaskHeader.tsx(Main Chat View):Testing
Unit Tests Added
ModeBadge Component (5 tests):
TaskItemHeader (2 new tests):
TaskHeader (2 new tests):
Total: 19 tests passing ✅
Manual Testing Completed
Screenshots
Note: Manual testing confirmed all visual requirements are met. Mode badges integrate seamlessly with the existing UI using VSCode theme colors.
Technical Notes
No Translation Changes: The implementation only displays mode names which are already defined in the mode configurations. No new user-facing strings were added.
CSS Variables: Uses existing VSCode badge CSS variables (
--vscode-badge-backgroundand--vscode-badge-foreground) that were already defined in the codebase.Performance: Minimal impact - the component is lightweight and only renders when a mode is present.
Accessibility: The badges inherit text color and background from VSCode theme, ensuring proper contrast. Tooltips provide additional context for truncated names.
Review Notes
During internal review, a critical test mock issue was identified and fixed in
TaskHeader.spec.tsx. The mock variable was improperly scoped, which could have caused test instability. This has been resolved using proper Vitest mocking patterns withvi.hoisted().Checklist
Important
Adds
ModeBadgecomponent to display mode indicators on task cards in chat and history views, with unit tests for new functionality.ModeBadgecomponent inModeBadge.tsxto display mode names on task cards.ModeBadgeinTaskHeader.tsxandTaskItemHeader.tsxto show mode indicators.ModeBadgeinModeBadge.spec.tsx.TaskHeader.spec.tsxandTaskItemHeader.spec.tsxto verify mode badge display.ModeBadge.This description was created by
for 7612005. You can customize this summary. It will automatically update as commits are pushed.