EnxDev commented on PR #41386: URL: https://github.com/apache/superset/pull/41386#issuecomment-4834279677
## EnxDev's Review Agent — apache/superset#41386 · HEAD 70843d6 lgtm (comment) — correct root-cause fix; aggregation is now part of change detection, and the save/restore path already round-trips `aggFunc`. Still a draft. I traced the full flow to confirm the fix is complete, not just the signature: - **Save** — `handleGridStateChange` persists `stateToSave.columnState = api.getColumnState()` (`AgGridTable/index.tsx:321,335`), which already carries `aggFunc`. The only gap was detection: the old signature hashed `columnOrder + sorts + filters`, so an aggFunc-only change never tripped `stateHash !== lastCapturedStateRef.current` and no save fired. Adding `aggregations` fixes exactly that. - **Trigger** — `handleGridStateChange` is wired to `onStateUpdated` (`index.tsx:520`), which fires on aggregation changes, so the new save path is reachable. - **Restore** — `reconcileColumnState` preserves the full state objects (incl. `aggFunc`), and `applyColumnState` re-applies `aggFunc: null` as "None" on reload. The round-trip holds. The `?? null` normalization correctly collapses the `null`/`undefined` "None" forms to one stable, distinct sentinel. ### 🟡 Should-fix - **`getColumnStateSignature.ts:48`** — `JSON.stringify` silently drops a *function* `aggFunc` (custom aggregator), collapsing it to the same signature as "None". Harmless today since Superset uses string aggFuncs (sum/avg/min/max), but if a function aggregator is ever wired in, its changes won't persist. Worth a one-line guard (`typeof col.aggFunc === 'function' ? 'custom' : (col.aggFunc ?? null)`) or a comment noting the string-only assumption. ### 🔵 Nits - `test/utils/getColumnStateSignature.test.ts:20,34` — `any[]`/`as any` throughout. Lint passes, but typing the fixtures as `ColumnState[]` would catch field-name drift (e.g. a future `aggFunc`→`aggregationFunc` rename) that `as any` hides. - Coverage: codecov flags the new `getColumnStateSignature(...)` call in `index.tsx` as the one uncovered patch line. The helper's unit tests prove the signature logic, but nothing exercises the save→reconcile→`applyColumnState` round-trip — i.e. the actual user-visible "None survives reload" behavior. A component/integration test asserting `aggFunc` is re-applied after restore would guard the real regression, not just the hash. ### 🙌 Praise - `getColumnStateSignature.ts` — clean extraction with a docstring that states the root cause; the regression test covering both `null` and `undefined` "None" forms is the right guard. <!-- enxdev-review-agent:70843d6 --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 70843d6._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
