Skip to content

Conversation

@eschutho
Copy link
Member

@eschutho eschutho commented Dec 20, 2025

User description

SUMMARY

This PR introduces interactive table capabilities inspired by Omni's data curation approach, where the table becomes a query-building interface rather than just output.

Key Features:

Query Tab in Explore (76c49ef)

Adds a new "Query" tab in the Data panel (alongside Results and Samples)
Displays the generated SQL with syntax highlighting
Auto-updates when form data changes, providing real-time SQL visibility
Column Header Context Menu for Standard Table (a1ea1af)

Right-click context menu on column headers in the standard Table chart
Options: "Group by this column", "Add as metric" (SUM/AVG/COUNT/etc.), "Hide column"
Uses setControlValue hook to update Explore controls
Column Header Interactions for AG Grid Table (4b2e6d1)

Extends AG Grid's existing kebab menu with the same interactive options
Group by, Add as metric (with submenu for aggregates), Hide column
Only appears in Explore context (when setControlValue is available)
How It Works:

Users can click on column headers to add Group By dimensions or create aggregate metrics
The Query tab shows the SQL updating in real-time as users interact with the table
This creates a bidirectional workflow: see data → click to refine → see SQL change → repeat

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: Tables are output-only; users must manually configure controls to change the query

After:

Column headers have context menus for quick query modification
New Query tab shows the generated SQL that updates as you interact with the table

TESTING INSTRUCTIONS

Navigate to Explore with any dataset
Select either "Table" or "AG Grid Table" visualization
Run a query with some columns
Test Query Tab:
4. Click on the "Query" tab in the data panel (next to Results/Samples)
5. Verify SQL is displayed with syntax highlighting
6. Change a control (e.g., add a metric) and verify SQL updates

Test Column Interactions (Standard Table):
7. Right-click on a dimension column header
8. Click "Group by this column" - verify column is added to Group By control
9. Right-click on a numeric column, hover "Add as metric", select "SUM"
10. Verify a new SUM metric appears in the Metrics control
11. Click "Hide column" and verify the column is hidden

Test Column Interactions (AG Grid Table):
12. Switch to AG Grid Table visualization
13. Click the kebab menu (⋮) on a column header
14. Verify "Group by this column", "Add as metric", and "Hide column" options appear
15. Test each option and verify controls update correctly

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration
  • Introduces new feature or API
  • Removes existing feature or API

Technical Notes:

Uses setControlValue from chart hooks to update Explore controls from within chart plugins
Uses getChartDataRequest with resultType: 'query' to fetch generated SQL
Column interactions are only visible in Explore context (not on dashboards)


CodeAnt-AI Description

Show generated SQL and enable column-based query edits from table headers

What Changed

  • Adds a new "Query" tab in the data panel that fetches and displays the generated SQL for the current chart, with syntax highlighting and a Copy button; it loads when the panel is open and the query is run.
  • Adds context-menu interactions on table and AG Grid column headers that let users directly modify the underlying query: "Group by this column", "Add as metric" (SUM/AVG/MIN/MAX/COUNT/COUNT DISTINCT), and "Hide column". Numeric-only aggregates are shown for numeric columns.
  • Column interactions only appear when Explore controls are available and update the chart controls (groupby, metrics, column_config), causing the query to refresh; the metric submenu is shown as a hover submenu.

Impact

✅ Real-time SQL visibility for charts
✅ Shorter query iteration by clicking table columns to add group-bys or metrics
✅ Hide columns from the table without opening the control panel

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Add a new "Query" tab to the DataTablesPane in Explore view that
displays the generated SQL query with syntax highlighting. This is
Phase 1 of making Explore more interactive by showing users the SQL
that powers their charts.

Features:
- New QueryPane component that fetches and displays generated SQL
- Syntax highlighting for SQL queries
- Copy to clipboard functionality
- Auto-updates when chart configuration changes
…lding

Add right-click context menu to table column headers that allows users
to modify the query directly from the table:

- Group by this column: Adds column to groupby control
- Add as metric: Creates SUM/AVG/COUNT/MIN/MAX aggregations
- Hide column: Updates column visibility

This enables Omni-style interactive table exploration where clicking
on columns updates the underlying query, visible in the new Query tab.

Features:
- New ColumnHeaderContextMenu component with Dropdown menu
- Passes setControlValue and formData through transformProps
- Supports numeric vs non-numeric column detection for appropriate aggregations
Extend AG Grid table's column header kebab menu with:
- Group by this column - adds column to groupby control
- Add as metric - submenu with SUM, AVG, MIN, MAX, COUNT, COUNT DISTINCT
- Hide column - hides column via column_config

These interactions only appear in Explore context (when setControlValue
is available from hooks). Clicking these options updates the Explore
controls which triggers a query refresh, allowing users to interactively
build queries by clicking on the table.

This is part of Phase 2 of interactive tables to compete with Omni-style
data curation surfaces.
@codeant-ai-for-open-source
Copy link
Contributor

CodeAnt AI is reviewing your PR.


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@dosubot dosubot bot added explore Namespace | Anything related to Explore viz:charts:table Related to the Table chart labels Dec 20, 2025
@codeant-ai-for-open-source codeant-ai-for-open-source bot added the size:XL This PR changes 500-999 lines, ignoring generated files label Dec 20, 2025
@eschutho eschutho added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Dec 20, 2025
@github-actions github-actions bot added 🎪 4b2e6d1 🚦 building Environment 4b2e6d1 status: building 🎪 4b2e6d1 📅 2025-12-20T05-08 Environment 4b2e6d1 created at 2025-12-20T05-08 🎪 4b2e6d1 🤡 eschutho Environment 4b2e6d1 requested by eschutho 🎪 ⌛ 48h Environment expires after 48 hours (default) and removed 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR labels Dec 20, 2025
@github-actions
Copy link
Contributor

🎪 Showtime is building environment on GHA for 4b2e6d1

@github-actions github-actions bot added 🎪 4b2e6d1 🚦 failed Environment 4b2e6d1 status: failed and removed 🎪 4b2e6d1 🚦 building Environment 4b2e6d1 status: building labels Dec 20, 2025
import Pagination from './components/Pagination';
import SearchSelectDropdown from './components/SearchSelectDropdown';
import { SearchOption, SortByItem } from '../types';
import { SearchOption, SortByItem, TableChartFormData } from '../types';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: The three identifiers SearchOption, SortByItem, and TableChartFormData are types; importing them as runtime values may cause bundling/runtime problems if they are exported only as types. Change this to a type-only import to ensure the import is erased during compilation. [type error]

Severity Level: Minor ⚠️

Suggested change
import { SearchOption, SortByItem, TableChartFormData } from '../types';
import type { SearchOption, SortByItem, TableChartFormData } from '../types';
Why it matters? ⭐

Those identifiers are used only as TypeScript types in this file (props and related types). Converting to import type { ... } removes unnecessary runtime imports and is the correct, idiomatic change.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
**Line:** 56:56
**Comment:**
	*Type Error: The three identifiers `SearchOption`, `SortByItem`, and `TableChartFormData` are types; importing them as runtime values may cause bundling/runtime problems if they are exported only as types. Change this to a type-only import to ensure the import is erased during compilation.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

import SearchSelectDropdown from './components/SearchSelectDropdown';
import { SearchOption, SortByItem } from '../types';
import { SearchOption, SortByItem, TableChartFormData } from '../types';
import { HandlerFunction } from '@superset-ui/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Importing HandlerFunction as a value import can cause runtime bundling/import issues if HandlerFunction is exported only as a TypeScript type (not a runtime value). Use a type-only import to ensure the import is erased at compile time and avoid potential runtime errors. [type error]

Severity Level: Minor ⚠️

Suggested change
import { HandlerFunction } from '@superset-ui/core';
import type { HandlerFunction } from '@superset-ui/core';
Why it matters? ⭐

This is a valid, useful change — HandlerFunction is only used as a prop type in this file, so using import type avoids emitting a runtime import and prevents potential bundling/runtime errors or warnings with type-only exports. It's safe and low-risk.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
**Line:** 57:57
**Comment:**
	*Type Error: Importing `HandlerFunction` as a value import can cause runtime bundling/import issues if `HandlerFunction` is exported only as a TypeScript type (not a runtime value). Use a type-only import to ensure the import is erased at compile time and avoid potential runtime errors.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

box-shadow: var(--ag-menu-shadow, ${theme.boxShadow});
border-radius: ${theme.borderRadius}px;
padding: ${theme.sizeUnit}px 0;
z-index: 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Hardcoded stacking order: the submenu uses a fixed z-index: 100 which can collide with other components; make it configurable via a CSS variable with a sensible fallback so it can be adjusted where the menu is used. [possible bug]

Severity Level: Critical 🚨

Suggested change
z-index: 100;
z-index: var(--ag-submenu-z-index, 100);
Why it matters? ⭐

Making the z-index configurable via a CSS variable (with a sensible fallback) prevents stacking collisions
with other components and is a low-risk improvement. The proposed improved code does exactly that.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx
**Line:** 157:157
**Comment:**
	*Possible Bug: Hardcoded stacking order: the submenu uses a fixed `z-index: 100` which can collide with other components; make it configurable via a CSS variable with a sensible fallback so it can be adjusted where the menu is used.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

ownState: serverPaginationData,
filterState,
hooks: { setDataMask = () => {}, onChartStateChange },
hooks: { setDataMask = () => {}, onChartStateChange, setControlValue },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Destructuring hooks directly from chartProps without a default for the parent object will throw if chartProps.hooks is undefined (TypeError: cannot destructure property of 'undefined'). Provide a default empty object for hooks in the destructuring so the nested defaults are applied safely. [null pointer]

Severity Level: Minor ⚠️

Suggested change
hooks: { setDataMask = () => {}, onChartStateChange, setControlValue },
hooks: { setDataMask = () => {}, onChartStateChange, setControlValue } = {},
Why it matters? ⭐

Accurate. If chartProps.hooks is undefined, destructuring its properties will throw at runtime. Defaulting the parent object (hooks = {}) is a small, defensive change that prevents a TypeError and is safe even if the type normally guarantees hooks exists.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
**Line:** 472:472
**Comment:**
	*Null Pointer: Destructuring `hooks` directly from `chartProps` without a default for the parent object will throw if `chartProps.hooks` is undefined (TypeError: cannot destructure property of 'undefined'). Provide a default empty object for `hooks` in the destructuring so the nested defaults are applied safely.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

formData,
chartState: serverPaginationData?.chartState,
onChartStateChange,
setControlValue,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: The returned props now include setControlValue (added to the returned object) but it may be undefined; returning an undefined value that callers will invoke can cause runtime errors. Ensure the returned property is always a function by coalescing to a noop when setControlValue is falsy. [possible bug]

Severity Level: Critical 🚨

Suggested change
setControlValue,
setControlValue: setControlValue ?? (() => {}),
Why it matters? ⭐

Valid: ensuring the returned prop is always a function (coalesce to a noop) prevents consumers from having to null-check before calling it. This duplicates the safety of providing a default during destructuring, but is a safe fallback if you can't change the destructuring.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
**Line:** 742:742
**Comment:**
	*Possible Bug: The returned props now include `setControlValue` (added to the returned object) but it may be undefined; returning an undefined value that callers will invoke can cause runtime errors. Ensure the returned property is always a function by coalescing to a noop when `setControlValue` is falsy.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

IHeaderParams,
CustomCellRendererProps,
} from '@superset-ui/core/components/ThemedAgGridReact';
import { HandlerFunction } from '@superset-ui/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Non-type import of HandlerFunction can emit a runtime import for a symbol that may only exist as a TypeScript type and not as a JS value; that can produce a runtime "undefined" import or bundler/runtime error. Import type-only declarations using import type to ensure no runtime import is emitted. [type error]

Severity Level: Minor ⚠️

Suggested change
import { HandlerFunction } from '@superset-ui/core';
import type { HandlerFunction } from '@superset-ui/core';
Why it matters? ⭐

HandlerFunction is only used in type positions in this file (interfaces), so switching to import type avoids emitting a runtime import for a symbol that is only a TS type. This reduces bundle ambiguity and is the correct modern TS usage.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts
**Line:** 46:46
**Comment:**
	*Type Error: Non-type import of `HandlerFunction` can emit a runtime import for a symbol that may only exist as a TypeScript type and not as a JS value; that can produce a runtime "undefined" import or bundler/runtime error. Import type-only declarations using `import type` to ensure no runtime import is emitted.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.


return (
<Dropdown
menu={{ items: menuItems }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: The Dropdown's menu items include per-item onClick handlers but the Dropdown/Menu API expects a top-level onClick in the menu prop (item-level onClick may be ignored depending on Ant Design version), so clicks won't invoke handleMenuClick. Add a top-level onClick that maps the clicked item's key to handleMenuClick. [logic error]

Severity Level: Minor ⚠️

Suggested change
menu={{ items: menuItems }}
menu={{
items: menuItems,
onClick: ({ key }) => handleMenuClick(key as MenuAction),
}}
Why it matters? ⭐

Ant Design's Dropdown/menu pattern relies on the menu's onClick handler to receive item clicks in many versions; embedding per-item onClick in the items array is unreliable across versions. Adding menu.onClick that maps keys to the existing handleMenuClick ensures clicks are always handled and fixes a real functional bug in this component.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-table/src/ColumnHeaderContextMenu.tsx
**Line:** 223:223
**Comment:**
	*Logic Error: The Dropdown's menu items include per-item `onClick` handlers but the `Dropdown`/`Menu` API expects a top-level `onClick` in the `menu` prop (item-level `onClick` may be ignored depending on Ant Design version), so clicks won't invoke `handleMenuClick`. Add a top-level `onClick` that maps the clicked item's `key` to `handleMenuClick`.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.


interface QueryResult {
query?: string;
language: SupportedLanguage;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Type/contract mismatch: QueryResult.language is typed as required but code defensively uses result.language || 'sql'; either mark language optional in the QueryResult interface or ensure the fetched results always set a valid SupportedLanguage. Make language optional to reflect runtime usage. [type error]

Severity Level: Minor ⚠️

Suggested change
language: SupportedLanguage;
language?: SupportedLanguage;
Why it matters? ⭐

The render uses result.language || 'sql', implying language may be undefined at runtime. Making language optional in the local interface aligns types with actual usage and avoids false positives from TypeScript. It's a small, correct relaxation of the contract.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/explore/components/DataTablesPane/components/QueryPane.tsx
**Line:** 96:96
**Comment:**
	*Type Error: Type/contract mismatch: `QueryResult.language` is typed as required but code defensively uses `result.language || 'sql'`; either mark `language` optional in the `QueryResult` interface or ensure the fetched results always set a valid `SupportedLanguage`. Make `language` optional to reflect runtime usage.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

Comment on lines +116 to +142
if (isRequest && isVisible && queryFormData) {
setIsLoading(true);
setError(null);

getChartDataRequest({
formData: queryFormData,
resultFormat: 'json',
resultType: 'query',
})
.then(({ json }) => {
setResults(ensureIsArray(json.result));
setError(null);
})
.catch(response => {
getClientErrorObject(response).then(({ error, message }) => {
setError(
error ||
message ||
response.statusText ||
t('An error occurred while fetching the query'),
);
});
})
.finally(() => {
setIsLoading(false);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: State update on unmounted component: the async fetch call can resolve after the component has unmounted and then call state setters (setResults, setError, setIsLoading), causing React warnings or memory leaks; guard state updates with an mounted flag and clean it up in the effect's return callback. [memory leak]

Severity Level: Minor ⚠️

Suggested change
if (isRequest && isVisible && queryFormData) {
setIsLoading(true);
setError(null);
getChartDataRequest({
formData: queryFormData,
resultFormat: 'json',
resultType: 'query',
})
.then(({ json }) => {
setResults(ensureIsArray(json.result));
setError(null);
})
.catch(response => {
getClientErrorObject(response).then(({ error, message }) => {
setError(
error ||
message ||
response.statusText ||
t('An error occurred while fetching the query'),
);
});
})
.finally(() => {
setIsLoading(false);
});
}
let isMounted = true;
if (isRequest && isVisible && queryFormData) {
setIsLoading(true);
setError(null);
getChartDataRequest({
formData: queryFormData,
resultFormat: 'json',
resultType: 'query',
})
.then(({ json }) => {
if (!isMounted) return;
setResults(ensureIsArray(json.result));
setError(null);
})
.catch(response => {
// getClientErrorObject returns a promise; guard updates after it resolves
getClientErrorObject(response).then(({ error, message }) => {
if (!isMounted) return;
setError(
error ||
message ||
response.statusText ||
t('An error occurred while fetching the query'),
);
});
})
.finally(() => {
if (isMounted) {
setIsLoading(false);
}
});
}
return () => {
isMounted = false;
};
Why it matters? ⭐

Valid suggestion. The effect performs async work and can call state setters after unmount. Introducing an isMounted/canceled flag (and guarding the promise resolution) prevents React warnings and pointless state updates. The proposed change is straightforward and safe.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/explore/components/DataTablesPane/components/QueryPane.tsx
**Line:** 116:142
**Comment:**
	*Memory Leak: State update on unmounted component: the async fetch call can resolve after the component has unmounted and then call state setters (`setResults`, `setError`, `setIsLoading`), causing React warnings or memory leaks; guard state updates with an mounted flag and clean it up in the effect's return callback.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

);
}

if (results.length === 0 || !results[0].query) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Logic bug: the component treats the results as "empty" when the first result has no query, even if later results contain queries; this will incorrectly render the EmptyState. Replace the check to verify that no result contains a query instead of only checking the first item. [logic error]

Severity Level: Minor ⚠️

Suggested change
if (results.length === 0 || !results[0].query) {
if (results.length === 0 || results.every(r => !r.query)) {
Why it matters? ⭐

This is a real logic bug: the current code treats the whole result set as empty if the first item lacks a query even when later results contain SQL. Replacing the check with results.every(r => !r.query) fixes that and aligns with getCurrentQuery which aggregates all results.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/explore/components/DataTablesPane/components/QueryPane.tsx
**Line:** 162:162
**Comment:**
	*Logic Error: Logic bug: the component treats the results as "empty" when the first result has no `query`, even if later results contain queries; this will incorrectly render the EmptyState. Replace the check to verify that no result contains a query instead of only checking the first item.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

@codeant-ai-for-open-source
Copy link
Contributor

CodeAnt AI finished reviewing your PR.

@codeant-ai-for-open-source
Copy link
Contributor

💡 Enhance Your PR Reviews

We noticed that 3 feature(s) are not configured for this repository. Enabling these features can help improve your code quality and workflow:

🚦 Quality Gates

Status: Quality Gates are not enabled at the organization level
Learn more about Quality Gates

🎫 Jira Ticket Compliance

Status: Jira credentials file not found. Please configure Jira integration in your settings
Learn more about Jira Integration

⚙️ Custom Rules

Status: No custom rules configured. Add rules via organization settings or .codeant/review.json in your repository
Learn more about Custom Rules


Want to enable these features? Contact your organization admin or check our documentation for setup instructions.

Copy link
Contributor

@bito-code-review bito-code-review bot left a comment

Choose a reason for hiding this comment

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

Code Review Agent Run #48c584

Actionable Suggestions - 1
  • superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/CustomHeader.tsx - 1
Additional Suggestions - 8
  • superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx - 2
    • Accessibility issue with submenu · Line 160-162
      The submenu is shown only on hover, which may not be accessible for keyboard users or those using assistive technologies. Consider adding focus styles or event handlers to ensure the submenu appears on focus or key navigation.
    • Custom CSS usage · Line 143-163
      The diff adds custom CSS styles for the submenu, which goes against the guideline to avoid custom CSS and follow antd best practices whenever possible. Since this is for ag-grid integration, it may be necessary, but consider if built-in options can be used.
  • superset-frontend/src/explore/components/DataTablesPane/components/QueryPane.tsx - 1
    • Async error handling bug · Line 129-137
      The catch block calls getClientErrorObject asynchronously without awaiting, risking unset error state if it fails, causing wrong UI (empty instead of error). Make it async and handle failures.
  • superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/CustomHeader.tsx - 1
    • Direct Ant Design import usage · Line 201-266
      The added column interaction menu items use direct imports from @ant-design/icons (GroupOutlined, CalculatorOutlined, EyeInvisibleOutlined), but codebase standards recommend using Icons from @superset-ui/core/components to avoid direct Ant Design imports.
  • superset-frontend/plugins/plugin-chart-table/src/ColumnHeaderContextMenu.tsx - 4
    • Unused icon imports · Line 24-31
      These icon imports are not referenced anywhere in the component, which keeps the code clean and reduces bundle size slightly.
    • Dead code in MenuAction type · Line 42-53
      These type variants and the filter case are not used in menuItems or elsewhere, representing dead code that should be cleaned up.
    • Unused theme variable · Line 62-62
      The theme variable is defined but never used in the component, so it can be removed along with its import.
    • Placeholder filter case · Line 114-117
      The filter case is a placeholder with no implementation, which might confuse future developers. Consider completing it or removing it.
Review Details
  • Files reviewed - 14 · Commit Range: 76c49ef..4b2e6d1
    • superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/CustomHeader.tsx
    • superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
    • superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx
    • superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx
    • superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
    • superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts
    • superset-frontend/plugins/plugin-chart-table/src/ColumnHeaderContextMenu.tsx
    • superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
    • superset-frontend/plugins/plugin-chart-table/src/transformProps.ts
    • superset-frontend/plugins/plugin-chart-table/src/types.ts
    • superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx
    • superset-frontend/src/explore/components/DataTablesPane/components/QueryPane.tsx
    • superset-frontend/src/explore/components/DataTablesPane/components/index.ts
    • superset-frontend/src/explore/components/DataTablesPane/types.ts
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

Comment on lines +132 to +169
const handleGroupBy = useCallback(() => {
if (!setControlValue || !formData) return;
const currentGroupby = ensureIsArray(formData.groupby);
if (!currentGroupby.includes(colId)) {
setControlValue('groupby', [...currentGroupby, colId]);
}
setMenuVisible(false);
}, [setControlValue, formData, colId]);

const handleAddMetric = useCallback(
(aggregate: string) => {
if (!setControlValue || !formData) return;
const currentMetrics = ensureIsArray(formData.metrics);
const metricLabel = `${aggregate}(${colId})`;
const newMetric = {
aggregate,
column: { column_name: colId },
expressionType: 'SIMPLE',
label: metricLabel,
};
setControlValue('metrics', [...currentMetrics, newMetric]);
setMenuVisible(false);
},
[setControlValue, formData, colId],
);

const handleHideColumn = useCallback(() => {
if (!setControlValue || !formData) return;
const currentColumnConfig = formData.column_config || {};
setControlValue('column_config', {
...currentColumnConfig,
[colId]: {
...currentColumnConfig[colId],
visible: false,
},
});
setMenuVisible(false);
}, [setControlValue, formData, colId]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing colId validation in handlers

The column interaction handlers use colId without checking if it's defined. Since colId = column?.getColId() can be undefined, this could add undefined to groupby or create invalid metrics, corrupting form data.

Code Review Run #48c584


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

@eschutho eschutho marked this pull request as draft December 20, 2025 06:11
Remove FilterOutlined, SortAscendingOutlined, SortDescendingOutlined,
and useTheme which were declared but never used.
@github-actions github-actions bot added 🎪 ab04af5 🚦 building Environment ab04af5 status: building 🎪 ab04af5 📅 2025-12-21T00-47 Environment ab04af5 created at 2025-12-21T00-47 labels Dec 21, 2025
@github-actions
Copy link
Contributor

🎪 Showtime is building environment on GHA for ab04af5

@github-actions github-actions bot added 🎪 ab04af5 🤡 eschutho Environment ab04af5 requested by eschutho 🎪 ab04af5 🚦 failed Environment ab04af5 status: failed and removed 🎪 ab04af5 🚦 building Environment ab04af5 status: building labels Dec 21, 2025
@github-actions github-actions bot removed 🎪 4b2e6d1 🤡 eschutho Environment 4b2e6d1 requested by eschutho 🎪 4b2e6d1 🚦 failed Environment 4b2e6d1 status: failed 🎪 4b2e6d1 📅 2025-12-20T05-08 Environment 4b2e6d1 created at 2025-12-20T05-08 🎪 ab04af5 🚦 failed Environment ab04af5 status: failed 🎪 ab04af5 📅 2025-12-21T00-47 Environment ab04af5 created at 2025-12-21T00-47 🎪 ab04af5 🤡 eschutho Environment ab04af5 requested by eschutho labels Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

explore Namespace | Anything related to Explore plugins size/XL size:XL This PR changes 500-999 lines, ignoring generated files viz:charts:table Related to the Table chart 🎪 ⌛ 48h Environment expires after 48 hours (default)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants