feat(ag-grid): support Show summary in raw records mode#41754
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Drop the rawSummaryColumns.length === 0 guard in the priming effect so a primed->empty transition writes the cleared list instead of leaving a stale prime behind. Gate the pinned totals row on totals being defined so raw-records mode with zero numeric columns renders no blank summary row, matching aggregate mode where a defined totals row is required. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Relocate show_totals to the top of Visual formatting and mark it renderTrigger so the whole section stays classified into the Customize tab. Since renderTrigger toggles skip the query cycle, add a symmetric ownState nudge (aggregateTotalsRequested) so switching the summary on in aggregate mode still re-queries for totals, mirroring the existing raw-records priming effect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two separate ownState effects (raw-mode summary priming and the aggregate-only totals nudge) each spread serverPaginationData in the same render, so they could clobber each other's writes. This let raw mode starve: toggling the summary off then back on with a prime that already matched the derived column list skipped both the priming write and the nudge, leaving totals unrequested and the row missing. Replace both effects with one that computes a single combined ownState delta covering priming and the totals request/clear for both query modes, and rename aggregateTotalsRequested to totalsRequested now that it applies to raw records too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code Review Agent Run #c05d36Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41754 +/- ##
==========================================
- Coverage 64.72% 64.67% -0.05%
==========================================
Files 2686 2686
Lines 148651 148594 -57
Branches 34309 34308 -1
==========================================
- Hits 96213 96108 -105
- Misses 50676 50731 +55
+ Partials 1762 1755 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both CodeAnt findings were valid; fixed in ca9d23c and 0cb8c56 (254/254 plugin tests passing).
One correction to the snippet proposed in that thread: clamping every
|
amaannawab923
left a comment
There was a problem hiding this comment.
left a few minor comments, rest of the pr looks good to me. mostly a small correctness nit and a test coverage ask, plus one non blocking question on the extra queries. nice work overall.
| } | ||
| cleanedTotals={totals || {}} | ||
| showTotals={showTotals} | ||
| showTotals={showTotals && totals !== undefined} |
There was a problem hiding this comment.
small correctness thing. this only guards against totals being undefined, but if an aggregate totals query comes back as an empty {} it would still pin a blank summary row, since {} !== undefined is true. we might want Object.keys(totals).length > 0 here to cover that case.
There was a problem hiding this comment.
Empty {} totals confirmed, and there is a concrete path to it: processComparisonTotals returns {} when the time-comparison totals result has no rows, which slipped past totals !== undefined and pinned a blank row. The gate is now showTotals && totals !== undefined && Object.keys(totals).length > 0, with a RED-first test (totals = {} → no pinned row). Note the SUM-over-zero-rows case still pins as intended: the backend returns a row with NULL values there, so the keys are present.
| // A renderTrigger toggle re-renders without re-querying; requesting totals | ||
| // through ownState dispatches the standard re-query whose buildQuery | ||
| // carries the totals query for the active mode. | ||
| if (showTotals && totals === undefined && !requested) { |
There was a problem hiding this comment.
not a blocker, just want to confirm the intent here. since the checkbox is a renderTrigger, a saved raw + summary chart does a priming re-query after the base load, and toggling off fires a query just to reset totalsRequested. its reasonable given buildQuery cant see column types and you already called it out in the description, so im mostly flagging the extra round trips, especially the reset only one, so its a conscious call. all good if the answer is yes, thats acceptable.
There was a problem hiding this comment.
yes, conscious call, with these bounds: a saved aggregate + summary chart does zero extra work (totals ride the initial formData-driven query; the nudge only fires in the renderTrigger gap). A saved raw + summary chart pays one priming re-query on first load — the base query is a server-side cache hit, only the totals query is new work. The toggle-off reset query is the deliberate trade-off: the alternative (clearing the flag when data arrives) would fire an extra query after every totals fetch, which is strictly worse; toggle-off stays visually instant regardless because the grid gate hides the row before the query returns.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the detailed follow ups. The empty totals guard and the calculated column test both look good, and the answer on the extra queries makes sense as a conscious trade off . LGTM !! @EnxDev |
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
Makes the Interactive Table (
ag-grid-table) "Show summary" option available and functional in Raw Records mode, and moves the checkbox into the Customize tab.Before, two independent gates made the summary aggregate-only: the control was declared with

visibility: isAggMode(so it never rendered in Raw Records — a value set in Aggregate mode silently persisted viaresetOnHide: falsebut was inert), andbuildQueryonly emitted the totals query whenqueryMode === Aggregate.After:
SUMper numeric column, computed across the full filtered dataset (row limit does not apply — the same semantics the option already has in Aggregate mode). Non-numeric cells stay blank. Only columns backed by a dataset (physical or calculated) column are summed; free-form SQL expression columns are excluded.Design decisions worth calling out:
buildQueryhas no datasource column metadata, sotransformPropsderives the numeric, dataset-backed subset of the raw selection and the chart mirrors it intoownState.rawSummaryColumns— the same mechanism the plugin already uses formetricSqlExpressions. The totals query is then a standard extra query (columns: [],row_limit: 0, SIMPLESUMadhoc metrics labeled with the column names), reusing the existing AG-Grid-filter exclusion (totalsExtras) logic. A raw chart with the summary enabled performs one priming re-query on first load (the base query is a cache hit; only the totals query is new work).renderTrigger. Explore classifies a section into the Customize tab only when every control in it hasrenderTrigger(ControlPanelsContainer.tsx); addingshow_totalsto Visual formatting without it would have relocated all nine formatting controls to the Data tab. SincerenderTriggertoggles don't re-query, the chart requests totals through the plugin's existing ownState re-query channel (totalsRequested) — the same sanctioned mechanism as server pagination. Toggle-on shows the standard loading state and issues the same query the old Data-tab checkbox produced; toggle-off hides the row instantly and issues one cache-friendly query to clear the request flag. All totals-related ownState writes are combined into a single delta per render to avoid same-render clobbering between the raw-mode prime and the re-query nudge.buildQueryadds no totals query, and the flag goes dormant — no pinned row is shown and no further queries are issued.plugin-chart-tablekeepsshow_totalsin the Data tab and aggregate-only; this PR intentionally changes only the AG Grid table plugin.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: in Raw Records mode the Customize tab has no "Show summary" option (it only appeared in the Data tab in Aggregate mode).
After: Customize → Visual formatting starts with "Show summary"; enabling it in Raw Records pins a SUM row.
(Screenshots to be attached.)
TESTING INSTRUCTIONS
AG_GRID_TABLE_ENABLEDfeature flag.birth_nameswithnum).npx jest plugins/plugin-chart-ag-grid-table(250 tests).ADDITIONAL INFORMATION
AG_GRID_TABLE_ENABLED🤖 Generated with Claude Code