Skip to content

fix(dashboard): row limit warning missing for non-table charts#39911

Open
msyavuz wants to merge 1 commit intomasterfrom
msyavuz/fix/chart-row-warning
Open

fix(dashboard): row limit warning missing for non-table charts#39911
msyavuz wants to merge 1 commit intomasterfrom
msyavuz/fix/chart-row-warning

Conversation

@msyavuz
Copy link
Copy Markdown
Member

@msyavuz msyavuz commented May 6, 2026

SUMMARY

Boolean short-circuit in SliceHeader makes countFromSecondQuery resolve to false for any non-table viz, and false != null is true, so Number(false) = 0 overrides the real rowcount fallback — every non-table chart loses the row-limit warning. Bug introduced in #37112.

-const isTableChart = formData.viz_type === 'table';
-const countFromSecondQuery =
-  isTableChart && secondQueryResponse?.data?.[0]?.rowcount;
+const isTableChart =
+  formData.viz_type === VizType.Table ||
+  formData.viz_type === VizType.TableAgGrid;
+const countFromSecondQuery = isTableChart
+  ? secondQueryResponse?.data?.[0]?.rowcount
+  : undefined;

Also widens the viz_type check to include ag-grid-table so server-paginated AG Grid charts read the rowcount sidecar query (same path the regular table uses). Same widening applied in ChartPills (explore row-count label) and Chart.tsx (CSV streaming threshold).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

image

TESTING INSTRUCTIONS

  1. Create a bar chart with row_limit=10 against a dataset with >10 rows.
  2. Add to a dashboard — warning icon should appear in the slice header.
  3. Repeat with an AG Grid table with server pagination and a row limit smaller than the dataset — warning should appear.

ADDITIONAL INFORMATION

  • Has associated issue: No
  • Required feature flags: None
  • Changes UI: Yes — restores row-limit warning icon on non-table charts
  • Includes DB Migration: No
  • Introduces new feature or API: No
  • Removes existing feature or API: No

Boolean short-circuit on `isTableChart && secondQueryResponse?.data?.[0]?.rowcount`
returned `false` for non-table viz types; `false != null` is `true`, so
`Number(false) = 0` became `sqlRowCount` and the warning never fired.
Also widens the table check to include AG Grid table so server-paginated
AG Grid charts read the rowcount sidecar query like the regular table.
@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review Bot commented May 6, 2026

Code Review Agent Run #0fb5fe

Actionable Suggestions - 0
Review Details
  • Files reviewed - 4 · Commit Range: 85b9802..85b9802
    • superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx
    • superset-frontend/src/dashboard/components/SliceHeader/index.tsx
    • superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx
    • superset-frontend/src/explore/components/ChartPills.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ 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 evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@dosubot dosubot Bot added the dashboard Namespace | Anything related to the Dashboard label May 6, 2026
Copy link
Copy Markdown
Contributor

@EnxDev EnxDev left a comment

Choose a reason for hiding this comment

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

LGTM!

@EnxDev EnxDev added the hold:testing! On hold for testing label May 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dashboard Namespace | Anything related to the Dashboard hold:testing! On hold for testing preset-io size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants