Skip to content

fix(ag-grid): persist value aggregation changes without a sort - #41388

Draft
amaannawab923 wants to merge 1 commit into
masterfrom
fix/97551-value-aggregation-persist
Draft

fix(ag-grid): persist value aggregation changes without a sort#41388
amaannawab923 wants to merge 1 commit into
masterfrom
fix/97551-value-aggregation-persist

Conversation

@amaannawab923

Copy link
Copy Markdown
Contributor

SUMMARY

Changing a metric's Value Aggregation on an interactive table and clicking Update chart didn't save — it quietly reverted to the previous option. The one workaround people found was to also trigger a sort on the column first and then update, which was a big hint about what was going on.

The grid's column-state change detection only keyed off column order, sorts and filters, so a standalone aggregation change wasn't captured at all. Triggering a sort changed the detected state and dragged the aggregation along with it — hence the workaround. Including the per-column aggregation in the change signature makes the aggregation change persist on its own.

This shares its root cause with the related "Value Aggregation None doesn't persist" fix — same signature gap, different symptom.

TESTING INSTRUCTIONS

  1. Create an interactive table using a dataset with a numeric metric (e.g. SUM(sales)).
  2. From the metric's Value Aggregation, pick a different option and click Update chart (without sorting first).
  3. The selected aggregation should be saved and survive a reload.

Unit tests added covering that an aggregation change alters the capture signature without any sort/order/filter change.

ADDITIONAL INFORMATION

  • Changes UI

…thout a sort (#97551)

Changing a column's Value Aggregation only persisted if another piece of grid
state (such as a sort) changed at the same time, because the grid-state change
detector hashed only column order, sorts and filters. This matches the
reported workaround (trigger a sort, then Update chart).

Include per-column aggFunc in the capture signature so the change is detected
and saved on its own. Same root cause as #107166.

Add unit tests for the Explore 'change aggregation then Update chart' scenario.
@amaannawab923 amaannawab923 changed the title fix(plugin-chart-ag-grid-table): persist value aggregation changes without a sort fix(ag-grid): persist value aggregation changes without a sort Jun 24, 2026
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.41%. Comparing base (a57b5f6) to head (da53bf7).
⚠️ Report is 755 commits behind head on master.

Files with missing lines Patch % Lines
...ugin-chart-ag-grid-table/src/AgGridTable/index.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #41388      +/-   ##
==========================================
- Coverage   64.42%   64.41%   -0.01%     
==========================================
  Files        2654     2655       +1     
  Lines      145386   145390       +4     
  Branches    33557    33558       +1     
==========================================
- Hits        93658    93657       -1     
- Misses      50031    50036       +5     
  Partials     1697     1697              
Flag Coverage Δ
javascript 68.72% <83.33%> (-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.

@EnxDev

EnxDev commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

EnxDev's Review Agent — #41388 · HEAD da53bf7

lgtm — Correct, minimal fix: the column-state change signature now folds in per-column aggFunc, so a standalone Value Aggregation change is captured on its own instead of needing a piggybacked sort. CI green; behavior preserved everywhere else.

Classified as a fix. The extraction of the inline JSON.stringify({columnOrder, sorts, filters}) into getColumnStateSignature is behavior-equivalent for the existing keys — the hash is only ever compared against itself (lastCapturedStateRef), so the added aggregations key and reordering can't cause a spurious or missed capture beyond the intended one. Initial-capture timing is unchanged; only later aggFunc changes now trigger an extra capture, which is the goal.

🟡 Should-fix

  • test/utils/getColumnStateSignature.test.ts — The aggFunc ?? null normalization is the most defensive line in the diff (without it, AG Grid reporting undefined one render and null the next for "no aggregation" would flip the signature and fire a spurious capture), but no test asserts it. Add a case: {aggFunc: undefined} and {aggFunc: null} for the same colId must produce the same signature. That's the regression guard for the normalization.

🔵 Nits

  • src/AgGridTable/index.tsx:338 — codecov flags the new call site as the 1 uncovered patch line; the util is unit-tested but the integration (capture actually fires on aggregation change) isn't. Low priority given the unit coverage, but a component-level test exercising handleGridStateChange would close the loop and directly guard #97551.

🙌 Praise

  • getColumnStateSignature.ts — clear docstring tying the change back to the observed symptom and the sort-first workaround; the ?? null sentinel reasoning is well-documented.

Note: PR is still a draft (review:draft), so no maintainer review is expected yet — flagging the test gap now so it's in before it goes ready.

Reviewed by EnxDev's Review Agent — @EnxDev · HEAD da53bf7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants