codeant-ai-for-open-source[bot] commented on code in PR #38791:
URL: https://github.com/apache/superset/pull/38791#discussion_r2971618962
##########
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx:
##########
@@ -49,148 +44,97 @@ const DetailsPanelPopover = ({
onHighlightFilterSource,
children,
popoverVisible,
- popoverContentRef,
- popoverTriggerRef,
setPopoverVisible,
}: DetailsPanelProps) => {
+ const theme = useTheme();
const activeTabs = useSelector<RootState>(
state => state.dashboardState?.activeTabs,
);
- // Combined ref array for all filter indicator elements
- const indicatorRefs = useRef<(HTMLButtonElement | null)[]>([]);
-
- const handleKeyDown = (event: KeyboardEvent<HTMLDivElement>) => {
- switch (event.key) {
- case 'Escape':
- case 'Enter':
- // timing out to allow for filter selection to happen first
- setTimeout(() => {
- // move back to the popover trigger element
- popoverTriggerRef?.current?.focus();
- // Close popover on ESC or ENTER
- setPopoverVisible(false);
- });
- break;
- case 'ArrowDown':
- case 'ArrowUp': {
- event.preventDefault(); // Prevent scrolling
- // Navigate through filters with arrows up/down
- const currentFocusIndex = indicatorRefs.current.findIndex(
- ref => ref === document.activeElement,
- );
- const maxIndex = indicatorRefs.current.length - 1;
- let nextFocusIndex = 0;
-
- if (event.key === 'ArrowDown') {
- nextFocusIndex =
- currentFocusIndex >= maxIndex ? 0 : currentFocusIndex + 1;
- } else if (event.key === 'ArrowUp') {
- nextFocusIndex =
- currentFocusIndex <= 0 ? maxIndex : currentFocusIndex - 1;
- }
- indicatorRefs.current[nextFocusIndex]?.focus();
- break;
- }
- case 'Tab':
- // forcing popover context until ESC or ENTER are pressed
- event.preventDefault();
- break;
- default:
- break;
- }
- };
-
- const handleVisibility = (isOpen: boolean) => {
- setPopoverVisible(isOpen);
- };
-
- // we don't need to clean up useEffect, setting { once: true } removes the
event listener after handle function is called
- useEffect(() => {
- if (popoverVisible) {
- window.addEventListener('resize', () => setPopoverVisible(false), {
- once: true,
- });
- }
- }, [popoverVisible]);
- // if tabs change, popover doesn't close automatically
useEffect(() => {
setPopoverVisible(false);
- }, [activeTabs]);
+ }, [activeTabs, setPopoverVisible]);
const indicatorKey = (indicator: Indicator): string =>
`${indicator.column} - ${indicator.name}`;
Review Comment:
**Suggestion:** Menu item keys are generated only from column and name,
which can collide when multiple indicators share the same label across
different paths; AntD Menu and React require unique keys, and collisions can
cause incorrect item rendering/focus behavior. Include path information in the
key to make it stable and unique. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Filter Badge menu items can collapse unexpectedly.
- ❌ Clicking indicator may target wrong filter source.
- ⚠️ Keyboard navigation becomes inconsistent in popover menu.
```
</details>
```suggestion
`${indicator.column} - ${indicator.name} - ${indicator.path?.join('>')
?? ''}`;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Native Filters config, create two filters with same display
name/column (name is
only required, not unique: `FiltersConfigForm.tsx:903-905`), and scope both
to the same
chart.
2. Apply both filters so `selectNativeIndicatorsForChart` emits two
indicators with
potentially identical `column`/`name` but distinct `path`
(`selectors.ts:69-77`), then
`FiltersBadge` collects them.
3. `FiltersBadge` intentionally keeps duplicate APPLIED indicators
(`uniqWith` comparator
at `FiltersBadge/index.tsx:266-273` does not dedupe when both are
`IndicatorStatus.Applied`).
4. `DetailsPanelPopover` builds menu item keys from only column+name
(`DetailsPanel/index.tsx:58-59`, used at `:92`), producing duplicate AntD
Menu keys; this
can cause merged/mis-targeted menu behavior and unstable keyboard navigation.
```
</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/FiltersBadge/DetailsPanel/index.tsx
**Line:** 59:59
**Comment:**
*Logic Error: Menu item keys are generated only from column and name,
which can collide when multiple indicators share the same label across
different paths; AntD Menu and React require unique keys, and collisions can
cause incorrect item rendering/focus behavior. Include path information in the
key to make it stable and unique.
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%2F38791&comment_hash=fbd563358dbb5cae42adb8707d3a14c85d59c544f0fcb5d5a99da3a7e3664205&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38791&comment_hash=fbd563358dbb5cae42adb8707d3a14c85d59c544f0fcb5d5a99da3a7e3664205&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]
