codeant-ai-for-open-source[bot] commented on PR #36855: URL: https://github.com/apache/superset/pull/36855#issuecomment-3696427187
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36855/files#diff-aeab17efd0df9e6433e1b0a7ca86dab44c3496e8a55b62b7d0a4af57b9254e12R189-R196'><strong>Possible Bug</strong></a><br>If a drag ends with no valid `over` target (user releases outside any tab), the current code will compute `overIndex` as -1 and still call `onTabsReorder(activeIndex, -1)`. This can produce invalid reorder operations or unexpected behavior downstream; the handler should be guarded against `over == null` (or `over.id == null`) and avoid calling reorder in that case.<br> - [ ] <a href='https://github.com/apache/superset/pull/36855/files#diff-8c118bd57aee3731e03d922e22c33f59bf1a4a0c977ca0d9427426565d791c95R165-R169'><strong>Ref forwarding risk</strong></a><br>The new `LineEditableTabs` export is typed as `FC<TabsProps>` and created via `Object.assign(StyledLineEditableTabs, { TabPane })`. That combination can hide the fact that the underlying styled component may not forward refs in the way callers expect. If consumers rely on refs (for DOM access or imperative Antd APIs), the typing may be incorrect and lead to runtime surprises. Confirm ref behavior and consider exporting a type that preserves ref semantics.<br> - [ ] <a href='https://github.com/apache/superset/pull/36855/files#diff-8c118bd57aee3731e03d922e22c33f59bf1a4a0c977ca0d9427426565d791c95R165-R169'><strong>Type casting masks mismatches</strong></a><br>The value is force-cast to `FC<TabsProps> & { TabPane: typeof StyledTabPane }` using `as`. This can mask incompatible prop or static property shapes between the styled component and the declared type. Verify that `StyledLineEditableTabs` truly accepts `TabsProps` and that `TabPane` static is interoperable with usages (including defaultProps).<br> - [ ] <a href='https://github.com/apache/superset/pull/36855/files#diff-aeab17efd0df9e6433e1b0a7ca86dab44c3496e8a55b62b7d0a4af57b9254e12R195-R200'><strong>Stale-closure / Dependency</strong></a><br>The `onDragEnd` callback uses `tabIdsRef.current` to read the latest ids (correct), but its dependency array still includes `tabIds`. This causes unnecessary re-creation of the callback when `tabIds` changes (and defeats some of the intended effect of the ref). Decide whether to rely on the ref (and remove `tabIds` from deps), or rely on `tabIds` directly and remove the ref — keep them consistent.<br> - [ ] <a href='https://github.com/apache/superset/pull/36855/files#diff-205b1ef25ad2e3e7574d5894c602a794e9c1dadb52020f116c215f986eaa33a4R352-R366'><strong>Unnecessary dependency / extra re-renders</strong></a><br>The dependency array for `handleTabsReorder` includes `activeKey` even though the callback body does not reference it. This will cause the callback to be recreated more often than needed, potentially triggering extra renders or re-registrations in children receiving the callback.<br> </td></tr> </table> -- 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]
