codeant-ai-for-open-source[bot] commented on code in PR #36855:
URL: https://github.com/apache/superset/pull/36855#discussion_r2650895705
##########
superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.tsx:
##########
@@ -173,9 +188,10 @@ const TabsRenderer = memo<TabsRendererProps>(
const onDragEnd = useCallback(
({ active, over }: DragEndEvent) => {
+ const currentTabIds = tabIdsRef.current;
if (active.id !== over?.id && onTabsReorder) {
- const activeIndex = tabIds.findIndex(id => id === active.id);
- const overIndex = tabIds.findIndex(id => id === over?.id);
+ const activeIndex = currentTabIds.findIndex(id => id === active.id);
+ const overIndex = currentTabIds.findIndex(id => id === over?.id);
onTabsReorder(activeIndex, overIndex);
}
Review Comment:
**Suggestion:** When a drag ends with no valid drop target (`over` is null)
or when either the dragged or target tab ID is not found in the current tab ID
list, `onTabsReorder` is still called with index `-1`, which can cause
incorrect reordering or downstream errors in callers that assume valid indices.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
// Only reorder when we have a valid drop target and both IDs are
found
if (!over || !onTabsReorder || active.id === over.id) {
setActiveId(null);
return;
}
const activeIndex = currentTabIds.findIndex(id => id === active.id);
const overIndex = currentTabIds.findIndex(id => id === over.id);
if (activeIndex !== -1 && overIndex !== -1) {
onTabsReorder(activeIndex, overIndex);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion correctly identifies a real logic bug in the PR: when `over`
is null (no drop target) or when either ID isn't found in `tabIdsRef.current`,
the current implementation still calls `onTabsReorder` with -1 for one or both
indices. The final file state shows exactly this behavior (it checks only
`active.id !== over?.id && onTabsReorder`), so the proposed guards (return
early if !over or same id, and ensure both indices !== -1 before calling)
prevent invalid reorder calls and avoid pushing -1 indices to callers. This is
a functional correctness fix, not just a stylistic change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.tsx
**Line:** 192:196
**Comment:**
*Logic Error: When a drag ends with no valid drop target (`over` is
null) or when either the dragged or target tab ID is not found in the current
tab ID list, `onTabsReorder` is still called with index `-1`, which can cause
incorrect reordering or downstream errors in callers that assume valid indices.
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>
##########
superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.jsx:
##########
@@ -362,7 +362,7 @@ const Tabs = props => {
setActiveKey(currentActiveTabId);
}
},
- [props.component, props.updateComponents, selectedTabIndex],
+ [props.component, props.updateComponents, selectedTabIndex, activeKey],
Review Comment:
**Suggestion:** The dependency array for the callback includes a state value
that is not used inside the function body; this causes `handleTabsReorder` to
be re-created on every active tab change, leading to unnecessary re-renders and
defeating some of the memoization benefit of `useCallback` without any
functional gain. [performance]
**Severity Level:** Minor ⚠️
```suggestion
[props.component, props.updateComponents, selectedTabIndex],
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
I inspected handleTabsReorder in the final file. The callback reads
props.component, props.updateComponents and selectedTabIndex, but never reads
the activeKey variable — it only calls setActiveKey(currentActiveTabId).
Including activeKey in the dependency array forces React to recreate the
callback whenever activeKey changes even though the callback's behavior does
not depend on that value. Removing activeKey from the deps is safe and reduces
unnecessary callback re-creations and potential re-renders of memoized children
that depend on this callback.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.jsx
**Line:** 365:365
**Comment:**
*Performance: The dependency array for the callback includes a state
value that is not used inside the function body; this causes
`handleTabsReorder` to be re-created on every active tab change, leading to
unnecessary re-renders and defeating some of the memoization benefit of
`useCallback` without any functional gain.
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>
--
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]
