EnxDev commented on PR #41851: URL: https://github.com/apache/superset/pull/41851#issuecomment-4966570105
## EnxDev's Review Agent — apache/superset#41851 · HEAD 759c72b **comment** — All three should-fix items from my prior review are resolved (verified in the diff). Core logic is correct. Two narrow edge cases and a couple of test-coverage gaps remain; none block. _Supersedes my [prior review](https://github.com/apache/superset/pull/41851#issuecomment-4961755514) on `535b2ec`._ **Prior findings — now fixed (confirmed in this revision):** - ag-grid UUID exclusion — `AgGridTableChart.tsx:114` now filters `col?.dataType === String && !isUuidColumn(col)`. The `type` propagation is no longer dead setup. - Wrapped/dialect UUID types — `isUuidColumn` uses `.includes('uuid')`, so `Nullable(UUID)` (ClickHouse) matches. - Stale persisted `searchColumn` — both handlers now validate it against `searchOptions` (`searchOptions.some(...)`) before reuse and fall back to `searchOptions[0]`. Also re-verified: legacy filter `visibleColumnsMeta[index]` (`TableChart.tsx:1487`) aligns 1:1 with the filtered `columns` (`= visibleColumnsMeta.map(getColumnConfigs)`, `:1434`), and `isEqualColumns` now keys on `` `${column_name}:${type}` `` so a type reassociation busts the memo. The bots' remaining "critical/major" comments are stale against the earlier `bd4cfa6` commit. ### 🟡 Should-fix - **`plugin-chart-table/src/transformProps.ts:325` (and ag-grid `:452`)** — `columnsByName` is keyed by `column_name`, so an adhoc/SQL-expression dimension over a UUID column gets `type: undefined`, stays `alphanumeric`, and remains searchable → the same invalid `ILIKE`. Narrower than the native case and not a regression (pre-existing gap), but worth a code comment marking the limitation. ### 🔵 Nits - **`TableChart.tsx:1584` (and ag-grid handler)** — the stale-`searchColumn` reconciliation runs only inside the search handler, not on mount. A chart with a UUID `searchColumn` + `searchText` persisted before this fix would still fire one `ILIKE` on initial load. Very narrow; reconciling in the `searchOptions` effect would close it. - **Both new tests** use `type: 'UUID'` (exact) only. The `.includes('uuid')` handling of wrapped forms is now untested — add a `Nullable(UUID)` case so a future revert to `=== 'uuid'` is caught. - The `searchColumn` reconciliation branch has no test; a case where the persisted `searchColumn` is a now-excluded UUID and resets to `searchOptions[0]` would guard it. ### 🙌 Praise - Clean turnaround on the prior review — every flagged item addressed, plus the `column_name:type` memo key and stale-selection reset are the kind of details that are easy to skip. `aria-label={t('Search by')}` also makes the Search By control properly queryable/accessible. <!-- enxdev-review-agent:759c72b --> _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 759c72b._ -- 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]
