Re: [PR] chore: Upgrade to React 18 [superset]

["codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>]

codeant-ai-for-open-source[bot] commented on code in PR #38563:
URL: https://github.com/apache/superset/pull/38563#discussion_r3078981027


##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -299,10 +299,12 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo(
     };
 
     const handleColumnHeaderClick = useCallback(
-      params => {
+      (params: { column?: { colId?: string; sort?: string } }) => {
         const colId = params?.column?.colId;
         const sortDir = params?.column?.sort;
-        handleColSort(colId, sortDir);
+        if (colId && sortDir) {
+          handleColSort(colId, sortDir);

Review Comment:
   **Suggestion:** The new click handler only calls sorting when `sortDir` is truthy, which skips the `null` sort case used to clear sorting from the custom header. This prevents users from unsorting a column and leaves stale sort state in server pagination flows. Allow `null` to pass through (while still ignoring `undefined`) so clear-sort events are handled. [logic error]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ⚠️ Table V2 server-sorted columns cannot be fully unsorted.
   - ⚠️ ServerPagination sortBy state remains stale after clear sort.
   - ⚠️ UI header state diverges from backend sorting behavior.
   ```
   </details>
   
   ```suggestion
         (params: { column?: { colId?: string; sort?: string | null } }) => {
           const colId = params?.column?.colId;
           const sortDir = params?.column?.sort;
           if (colId && sortDir !== undefined) {
             handleColSort(colId, sortDir as string);
   ```
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Open any "Table V2" (AG Grid Table) visualization in Explore or on a dashboard; this
   uses `TableChart` from
   `superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:4-38`,
   which renders `<AgGridDataTable>` at `AgGridTableChart.tsx:29-52`.
   
   2. Ensure the chart is configured with server-side pagination so the `serverPagination`
   prop is true and `serverPaginationData` is populated (these props are passed to
   `<AgGridDataTable>` at `AgGridTableChart.tsx:25-32` and consumed in
   `AgGridTable/index.tsx:133-139, 248-255`).
   
   3. In the rendered grid, click a sortable column header multiple times to cycle sorting:
   the custom header component `CustomHeader`
   (`AgGridTable/components/CustomHeader.tsx:103-120`) calls `applySort`/`clearSort`; when
   clearing, `clearSort` sends `onColumnHeaderClicked({ column: { colId: sortKey, sort: null
   } })` as seen at `CustomHeader.tsx:25-27`.
   
   4. That callback is wired to `handleColumnHeaderClick` in `AgGridTable/index.tsx:301-308`,
   but the current implementation only calls `handleColSort(colId, sortDir)` when `if (colId
   && sortDir)` is truthy; a `sort` value of `null` from `clearSort` fails this check, so
   `handleColSort` is never invoked, its `sortDir == null` branch at `index.tsx:262-269`
   (which should reset AG Grid state and call `onSortChange([])`) never runs, and
   `handleSortByChange` in `AgGridTableChart.tsx:348-17` never receives an empty `sortBy`
   array—leaving stale server-side sort state even though the header UI shows "Clear Sort"
   was applied.
   ```
   </details>
   <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/AgGridTable/index.tsx
   **Line:** 302:306
   **Comment:**
   	*Logic Error: The new click handler only calls sorting when `sortDir` is truthy, which skips the `null` sort case used to clear sorting from the custom header. This prevents users from unsorting a column and leaves stale sort state in server pagination flows. Allow `null` to pass through (while still ignoring `undefined`) so clear-sort events are handled.
   
   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.
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38563&comment_hash=df6f935ed45c06bbb741a9d1b01fb0295e70b98bb9f784a25873e0461a9f0e9a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38563&comment_hash=df6f935ed45c06bbb741a9d1b01fb0295e70b98bb9f784a25873e0461a9f0e9a&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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org