codeant-ai-for-open-source[bot] commented on code in PR #38791:
URL: https://github.com/apache/superset/pull/38791#discussion_r3008940416
##########
superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx:
##########
@@ -243,28 +245,15 @@ test('Arrow key navigation switches focus between
indicators', () => {
);
// Query the indicators
- const firstIndicator = screen.getByRole('button', {
- name: 'search Clinical Stage',
- });
- const secondIndicator = screen.getByRole('button', {
- name: 'search Age Group',
- });
+ const firstMenuItem = screen.queryByText('Clinical Stage')?.closest('li')!;
+ const secondMenuItem = screen.queryByText('Age Group')?.closest('li')!;
- // Focus the first indicator
- firstIndicator.focus();
- expect(firstIndicator).toHaveFocus();
+ expect(firstMenuItem).toBeInTheDocument();
+ expect(secondMenuItem).toBeInTheDocument();
- // Simulate ArrowDown key press
- fireEvent.keyDown(document.activeElement as Element, {
- key: 'ArrowDown',
- code: 'ArrowDown',
- });
- expect(secondIndicator).toHaveFocus();
+ userEvent.type(firstMenuItem, '{arrowdown}');
+ expect(firstMenuItem).toHaveFocus();
- // Simulate ArrowUp key press
- fireEvent.keyDown(document.activeElement as Element, {
- key: 'ArrowUp',
- code: 'ArrowUp',
- });
- expect(firstIndicator).toHaveFocus();
+ userEvent.type(secondMenuItem, '{arrowdown}');
Review Comment:
**Suggestion:** The "Arrow key navigation switches focus between indicators"
test never actually verifies that focus moves between menu items when using the
arrow keys; instead it types ArrowDown on each item and asserts that focus
stays on the same element, which contradicts the intended keyboard navigation
behavior and can let regressions slip through undetected. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Jest test misses regressions in DetailsPanel keyboard navigation
behavior.
- ⚠️ Dashboard filter badge popover a11y relies on untested arrows.
- ⚠️ Manual QA must catch arrow-key focus regressions instead.
```
</details>
```suggestion
const firstMenuItem = screen.getByText('Clinical Stage').closest('li') as
HTMLElement;
const secondMenuItem = screen.getByText('Age Group').closest('li') as
HTMLElement;
expect(firstMenuItem).toBeInTheDocument();
expect(secondMenuItem).toBeInTheDocument();
// Focus the first item and use keyboard navigation to move focus
firstMenuItem.focus();
expect(firstMenuItem).toHaveFocus();
await userEvent.keyboard('{ArrowDown}');
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx`
and locate the test `Arrow key navigation switches focus between indicators`
at lines
221–259.
2. Observe that the test queries the two indicators' menu items via
`screen.queryByText('Clinical Stage')?.closest('li')` and
`screen.queryByText('Age
Group')?.closest('li')` (lines 248–249), then calls
`userEvent.type(firstMenuItem,
'{arrowdown}')` and asserts `expect(firstMenuItem).toHaveFocus()` (lines
254–255), and
similarly for `secondMenuItem` (lines 257–258).
3. Note that `userEvent.type(target, '{arrowdown}')` focuses the `target`
element as part
of the helper behavior, so the expectations only confirm that the element
passed as
`target` ends up focused, regardless of any keyboard navigation logic in
`DetailsPanelPopover`.
4. Run the tests for this file (for example, `npm test --
DetailsPanel.test.tsx`); the
`Arrow key navigation switches focus between indicators` test will pass even
if pressing
ArrowDown while focused on the first indicator does not move focus to the
second
indicator, because the test never checks that focus leaves the first menu
item and is
managed by the `Menu` component in
`superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx`
(lines
61–100).
```
</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/DetailsPanel.test.tsx
**Line:** 248:257
**Comment:**
*Logic Error: The "Arrow key navigation switches focus between
indicators" test never actually verifies that focus moves between menu items
when using the arrow keys; instead it types ArrowDown on each item and asserts
that focus stays on the same element, which contradicts the intended keyboard
navigation behavior and can let regressions slip through undetected.
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=49b2f9f5f3c0b0803099d2ed9e24e8b4fc8306800c451b9fcf5540abef9fbc4a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38791&comment_hash=49b2f9f5f3c0b0803099d2ed9e24e8b4fc8306800c451b9fcf5540abef9fbc4a&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]
