codeant-ai-for-open-source[bot] commented on code in PR #41851: URL: https://github.com/apache/superset/pull/41851#discussion_r3543739271
########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts: ########## @@ -360,6 +360,10 @@ const processColumns = memoizeOne(function processColumns( const percentMetricsSet = new Set(percentMetrics); const rawPercentMetricsSet = new Set(rawPercentMetrics); + const columnsByName = new Map( + (props.datasource.columns ?? []).map(col => [col.column_name, col]), + ); Review Comment: **Suggestion:** This introduces a dependency on `datasource.columns` inside a memoized `processColumns`, but the memoization comparator (`isEqualColumns`) does not include `datasource.columns`, so updated column metadata can be ignored and stale column `type` values can persist. Include `datasource.columns` in the equality check (or remove this memoization for type derivation) so UUID/type changes are reflected correctly. [cache] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ AG Grid table can show outdated column type metadata. - ⚠️ UUID detection relying on `columns.type` can be incorrect. - ⚠️ Server-side search may misclassify updated UUID columns. - ⚠️ Dataset schema changes not reflected in table behavior. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render an AG Grid table chart that uses `transformProps` in `superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts` (memoized `processColumns` at lines 18-32 and call site at lines 26-27) with some initial dataset column metadata in `props.datasource.columns`. 2. Later, update the dataset in Superset so that the native column type for one of the table columns changes (for example, mark a string ID column as a UUID); this updates `props.datasource.columns` and flows into `columnsByName` (lines 44-46) but leaves all fields checked by `isEqualColumns` in `superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/isEqualColumns.ts` (lines 41-68) unchanged. 3. When the chart re-renders, `memoizeOne(processColumns, isEqualColumns)` sees the new `chartProps` as equal to the previous props because `isEqualColumns` compares `datasource.columnFormats`, `currencyFormats`, `verboseMap`, `formData.metrics`, `queriesData[0].colnames`, `coltypes`, etc., but never inspects `datasource.columns` (lines 51-60), so it returns the cached `[metrics, percentMetrics, columns]` instead of recomputing. 4. The cached `columns` array still contains old `type` metadata set at `transformProps.ts` lines 125-135 (`type: columnsByName.get(key)?.type`), so downstream consumers of `AgGridTableChartTransformedProps.columns` (for example logic that will exclude UUIDs from search or display type-specific behavior) operate on stale type information and continue treating the modified UUID column as a generic string. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8df1cb1ead1f4d8b8598ee72ad5c3cf3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8df1cb1ead1f4d8b8598ee72ad5c3cf3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts **Line:** 363:365 **Comment:** *Cache: This introduces a dependency on `datasource.columns` inside a memoized `processColumns`, but the memoization comparator (`isEqualColumns`) does not include `datasource.columns`, so updated column metadata can be ignored and stale column `type` values can persist. Include `datasource.columns` in the equality check (or remove this memoization for type derivation) so UUID/type changes are reflected correctly. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41851&comment_hash=261c655cc2a5e581667b3ac4bd92838c1f03abff45aa97e4ac3b5dff18697d76&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41851&comment_hash=261c655cc2a5e581667b3ac4bd92838c1f03abff45aa97e4ac3b5dff18697d76&reaction=dislike'>👎</a> -- 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]
