fix(dashboard): let CSV exports use query cache instead of always force-querying

[eschutho]

SUMMARY

Two bugs combine to make Dashboard CSV/XLSX exports always re-execute the query instead of serving from cache, while the Explore page correctly hits the cache.

Bug 1 — Chart.tsx: The exportChart() call in the dashboard chart component hardcodes force: true, which tells the backend to bypass the cache entirely on every export. The Explore page does not set this flag, which is why it correctly uses the cache.

Bug 2 — buildQuery.ts: For download queries (result_format: 'csv' / 'xlsx'), row_limit was coerced to 0 via Number(formDataCopy.row_limit) || 0 when the chart has no explicit row_limit. Display queries leave row_limit as undefined. This produces a different cache key for the export request versus the display request that populated the cache, so the export always misses — even after fixing Bug 1.

Both fixes are required together:

• Removing force: true lets the backend consult the cache instead of always executing.
• Preserving undefined (instead of 0) for a missing row_limit ensures the export query generates the same cache key as the display query that already populated the cache.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable — backend cache behavior change with no UI change.

TESTING INSTRUCTIONS

1. Open a dashboard containing a Table chart backed by a slow query.
2. Wait for the chart to finish loading (it will be cached at this point).
3. Use the chart's kebab menu → Download → CSV. With this fix the export should return immediately (from cache); without it, the query is re-executed from scratch.
4. Confirm the same behavior on the Explore page (should continue to work as before).
5. Verify that explicit row_limit values on the chart are still respected in the export.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Code Review Agent Run #f351e6

Actionable Suggestions - 0

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

• superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx - 1

  • Export caching behavior change · Line 565-565

Review Details

• Files reviewed - 2 · Commit Range: 57b9305..57b9305

  • superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts
  • superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx
• 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 evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codeant-ai-for-open-source]

Suggestion: The new conversion only checks for null/undefined before calling Number, so non-numeric string values (for example legacy or URL-provided row_limit) become NaN here. That bypasses the normal buildQueryObject sanitization and injects an invalid row_limit into download queries, which can serialize differently from the display query and still miss cache (and may produce incorrect backend handling). Normalize invalid numeric values to undefined instead of passing through NaN. [cache]

Severity Level: Major ⚠️

❌ Table chart CSV/XLSX exports may still miss cache.
⚠️ Malformed row_limit values yield inconsistent export query objects.
⚠️ Edge-case exports may ignore intended row limits.

Steps of Reproduction ✅

1. In a unit test, import `buildQuery` from
`superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts` (function defined around
line 57) and call it with a `TableChartFormData` object where `result_format: 'csv'`,
`result_type: 'results'`, and `row_limit: 'not-a-number'` (a non-numeric string), plus any
valid datasource/metrics.

2. During `buildQuery` execution, the callback passed to `buildQueryContext` computes
`isDownloadQuery` (lines ~75–78) which evaluates to `true` for `result_format: 'csv'`,
then enters the `if (isDownloadQuery)` block at line 229.

3. Inside that block (lines 230–233) the code sets `moreProps.row_limit =
formDataCopy.row_limit != null ? Number(formDataCopy.row_limit) : undefined;`. With
`row_limit: 'not-a-number'`, `formDataCopy.row_limit != null` is `true`,
`Number('not-a-number')` produces `NaN`, so the resulting query object in
`buildQueryContext(...).queries[0]` has `row_limit` set to `NaN`.

4. The same chart’s on-screen (display) query, built with identical form data but
`result_format: 'json'` / non-download settings, does not pass through this
`isDownloadQuery` block and instead uses the sanitized `row_limit` from the underlying
`buildQueryContext`/`QueryObject` creation, so its `row_limit` is a valid integer or
`undefined`, not `NaN`. Because the backend cache keys for SQLA datasets are derived by
passing the query object into `get_sqla_query` with `SQLA_QUERY_KEYS` including
`"row_limit"` (see `superset/connectors/sqla/models.py:2131–2137` and
`superset/models/helpers.py:197–202` where `"row_limit"` is listed), the export query
containing `row_limit: NaN` (serialized to `null`) produces a different cache key than the
display query’s `row_limit` (valid int or omitted), so CSV/XLSX exports for such malformed
`row_limit` values will bypass the populated cache and re-execute the query. Normalizing
invalid numeric inputs to `undefined` here would keep export and display query shapes
aligned and restore cache reuse.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts
**Line:** 230:233
**Comment:**
	*Cache: The new conversion only checks for null/undefined before calling `Number`, so non-numeric string values (for example legacy or URL-provided `row_limit`) become `NaN` here. That bypasses the normal `buildQueryObject` sanitization and injects an invalid `row_limit` into download queries, which can serialize differently from the display query and still miss cache (and may produce incorrect backend handling). Normalize invalid numeric values to `undefined` instead of passing through `NaN`.

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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix

👍 | 👎

[bito-code-review]

The flagged issue is correct. When formDataCopy.row_limit is a non-numeric string, Number() returns NaN, which is then assigned to moreProps.row_limit. This causes inconsistent query objects and cache misses. To resolve this, you should check if the result of Number() is a valid number (i.e., not NaN) before assigning it, or default to undefined if it is invalid.

Here is the corrected implementation for superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:

      const rowLimit = Number(formDataCopy.row_limit);
      moreProps.row_limit =
        formDataCopy.row_limit != null && !Number.isNaN(rowLimit)
          ? rowLimit
          : undefined;

There are no other review comments in this pull request to address.

superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts

const rowLimit = Number(formDataCopy.row_limit);
      moreProps.row_limit =
        formDataCopy.row_limit != null && !Number.isNaN(rowLimit)
          ? rowLimit
          : undefined;

[codecov]

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.52%. Comparing base (dc64716) to head (57b9305).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
...ntend/plugins/plugin-chart-table/src/buildQuery.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #41469      +/-   ##
==========================================
- Coverage   64.52%   64.52%   -0.01%     
==========================================
  Files        2664     2664              
  Lines      146178   146180       +2     
  Branches    33722    33723       +1     
==========================================
  Hits        94319    94319              
- Misses      50142    50144       +2     
  Partials     1717     1717              

Flag	Coverage Δ
javascript	68.96% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.