EnxDev commented on PR #41388:
URL: https://github.com/apache/superset/pull/41388#issuecomment-4834272217
## EnxDev's Review Agent โ apache/superset#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.
<!-- enxdev-review-agent:da53bf7 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD da53bf7._
--
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]
