Skip to content

Conversation

@tianzhou
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings December 26, 2025 08:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a multi-tab result interface that allows users to view multiple query results simultaneously instead of replacing the previous result with each new execution. Each tab displays a timestamp, result/error status, and can be individually closed.

Key changes:

  • Refactored state management from single result to array of result tabs
  • Each query execution (success or failure) creates a new tab with timestamp and execution details
  • Added tab navigation and close functionality

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
frontend/src/components/views/ToolDetailView.tsx Refactored state management to use resultTabs array instead of single result; added tab creation logic for both successful queries and errors; implemented tab close handler
frontend/src/components/tool/ResultsTabs.tsx New component that renders tabbed interface with timestamp labels, error indicators, close buttons, and displays active tab's results
frontend/src/components/tool/types.ts New type definition for ResultTab interface containing id, timestamp, result, error, executedSql, and executionTimeMs
frontend/src/components/tool/index.ts Added exports for ResultsTabs component and ResultTab type
frontend/src/components/icons/XIcon.tsx New icon component for tab close buttons

Comment on lines +61 to +72
<button
key={tab.id}
type="button"
onClick={() => onTabSelect(tab.id)}
className={cn(
'group flex items-center gap-1.5 px-3 py-1.5 text-sm whitespace-nowrap',
'border-b-2 -mb-px transition-colors cursor-pointer',
tab.id === activeTabId
? 'border-primary text-foreground'
: 'border-transparent text-muted-foreground hover:text-foreground'
)}
>
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The tab buttons are missing keyboard navigation support for arrow keys. Users expect to be able to navigate between tabs using Left/Right arrow keys when focused on a tab. Consider implementing ARIA tabs pattern with proper keyboard navigation.

Copilot uses AI. Check for mistakes.
Comment on lines +231 to +232
setResultTabs(prev => [newTab, ...prev]);
setActiveTabId(newTab.id);
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

There's potential for memory leaks as the tabs array grows unbounded. Each query execution adds a new tab to the beginning of the array, but there's no limit on the total number of tabs. Consider implementing a maximum tab limit or providing a "clear all" functionality to prevent memory issues during extended usage sessions.

Copilot uses AI. Check for mistakes.
Comment on lines 77 to 93
<span
role="button"
tabIndex={0}
onClick={(e) => {
e.stopPropagation();
onTabClose(tab.id);
}}
onKeyDown={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
e.stopPropagation();
onTabClose(tab.id);
}
}}
className="opacity-0 group-hover:opacity-100 hover:bg-muted rounded p-0.5 transition-opacity"
>
<XIcon className="w-3 h-3" />
</span>
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The close button implementation has accessibility issues. While it includes keyboard support (Enter and Space), it's missing a proper accessible label. Screen reader users won't know what this button does. Add an aria-label attribute to describe the action.

Copilot uses AI. Check for mistakes.
Comment on lines 77 to 93
<span
role="button"
tabIndex={0}
onClick={(e) => {
e.stopPropagation();
onTabClose(tab.id);
}}
onKeyDown={(e) => {
if (e.key === 'Enter' || e.key === ' ') {
e.stopPropagation();
onTabClose(tab.id);
}
}}
className="opacity-0 group-hover:opacity-100 hover:bg-muted rounded p-0.5 transition-opacity"
>
<XIcon className="w-3 h-3" />
</span>
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The close button span should use 'button' as the role instead of having role="button" on a span element while also using click handlers. For better accessibility and semantics, consider using an actual button element with type="button" instead of a span with role="button".

Copilot uses AI. Check for mistakes.
Comment on lines +264 to +267
const newTabs = prev.filter(tab => tab.id !== idToClose);

if (idToClose === activeTabId && newTabs.length > 0) {
const nextIndex = Math.min(index, newTabs.length - 1);
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

When the tab being closed is the active tab and there are remaining tabs, the next tab selection logic could be improved. Currently it uses Math.min(index, newTabs.length - 1), but this doesn't account for the case where the closed tab was at the end. Consider selecting the previous tab (index - 1) when closing the last tab, or the next tab (same index) when closing any other tab, for a more intuitive user experience.

Suggested change
const newTabs = prev.filter(tab => tab.id !== idToClose);
if (idToClose === activeTabId && newTabs.length > 0) {
const nextIndex = Math.min(index, newTabs.length - 1);
// If the tab to close is not found, return the previous state unchanged
if (index === -1) {
return prev;
}
const newTabs = prev.filter(tab => tab.id !== idToClose);
if (idToClose === activeTabId && newTabs.length > 0) {
// When closing the active tab:
// - if it was the last tab, select the previous tab
// - otherwise, select the tab that shifted into its position (the "next" tab)
const nextIndex = index >= newTabs.length ? newTabs.length - 1 : index;

Copilot uses AI. Check for mistakes.
>
<span>{formatTimestamp(tab.timestamp)}</span>
{tab.error && (
<span className="w-1.5 h-1.5 rounded-full bg-destructive" />
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The error indicator (red dot) on tabs with errors lacks an accessible label. Screen reader users won't know what this visual indicator means. Add an aria-label or use a visually hidden text element to describe that this tab contains an error.

Suggested change
<span className="w-1.5 h-1.5 rounded-full bg-destructive" />
<span
className="w-1.5 h-1.5 rounded-full bg-destructive"
role="img"
aria-label="This tab contains an error"
/>

Copilot uses AI. Check for mistakes.
@tianzhou tianzhou merged commit 5ac23f2 into main Dec 26, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants