EnxDev commented on PR #41386:
URL: https://github.com/apache/superset/pull/41386#issuecomment-4897001535
EnxDev's Review Agent — apache/superset#41386 · HEAD 5d576e4
lgtm — re-review superseding my prior review at 70843d6; all three prior
findings resolved and verified in the diff, not just claimed. Correct
root-cause fix.
Verified each prior finding against the current diff:
- Function aggFunc guard (was Should-fix) — ✅ getColumnStateSignature.ts now
maps typeof col.aggFunc === 'function' ? 'custom' : (col.aggFunc ?? null). The
author's follow-up correctly refined my premise: an unguarded function
serialized as a dropped key ({"colId":…}), not as null, so it never actually
collapsed onto "None" — but the guard is still right, since the signature no
longer depends on JSON.stringify silently dropping function values. A unit test
pins function-vs-None distinctness.
- Typed fixtures (was nit) — ✅ getColumnStateSignature.test.ts fixtures are
ColumnState[], all as any removed.
- Round-trip coverage (was nit) — ✅ split across two seams:
reconcileColumnState.test.ts pins aggFunc pass-through (incl. explicit null) on
the restore leg this repo owns; AgGridTableChart.test.tsx drives
restore→debounced-save end to end. The full save→apply→read-back genuinely
can't be asserted in-repo (aggregation state needs AG Grid's enterprise
SharedAggregation module; community modules always read aggFunc back as null) —
honestly documented in the test and follow-up.
Core fix unchanged and still correct: getColumnStateSignature
(AgGridTable/index.tsx:341) folds per-column aggFunc into the change-detection
hash, so an aggFunc-only edit now trips stateHash !==
lastCapturedStateRef.current and fires a save; the save (api.getColumnState())
and restore (reconcileColumnState → applyColumnState) legs already round-trip
aggFunc, as traced in the prior review. CI is green — codecov/patch 100%, all
jest shards pass.
🔵 Nits
- test/AgGridTableChart.test.tsx — toMatchObject({ aggFunc: null }) can't
distinguish fixed from broken (community modules always report aggFunc: null),
so it only guards that the save path emits the key, not that "None" survives.
The real regression guard is the getColumnStateSignature unit test, which is
solid. Fine as-is; the limitation is documented.
- PR still carries the review:draft label (not a GitHub draft) — worth
clearing before it's merge-ready.
🙌 Praise
- The follow-up that corrected my Should-fix premise (function → dropped
key, not a null-collision) rather than silently "fixing" it — backed by a
distinctness test — is the right way to handle a review note.
--
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]
