feat(autox): support selecting files by clicking rows in file explorer#7114
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughCentralizes row selection in two FileExplorer components by replacing per-cell inline Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/automl/frontend/src/app/components/common/FileExplorer/FileExplorer.tsx`:
- Around line 497-500: The overflow toggle's aria-label currently uses a generic
string in the ActionsColumn actionsToggle render (the MenuToggle inside
FileExplorer.tsx), making every row's menu indistinguishable to screen readers;
change the aria-label to include a file-specific identifier (e.g., the file name
or id available on the row data) so each MenuToggle becomes unique (for example
build the label using the row's name/id like `${file.name} menu`), updating the
actionsToggle callback where MenuToggle is rendered to read that property from
the row prop or surrounding scope.
- Around line 442-445: The row onClick currently always calls onSelect(null,
true) (in FileExplorer's onClick handler) which makes selection one-way and
allows clicks from nested interactive controls (e.g., folder-name link) to
bubble into selection; change the handler to accept the click event,
early-return if isUnselectable or if the click originated from an interactive
descendant (detect via event.target.closest('button, input, a, textarea, select,
[role="button"]') or a data attribute), and compute the new selection state by
toggling isSelected (call onSelect(null, !isSelected)); keep the isUnselectable
check and ensure other handlers like onViewDetails still receive their events
when interactive descendants are clicked.
In
`@packages/autorag/frontend/src/app/components/common/FileExplorer/FileExplorer.tsx`:
- Around line 497-500: The overflow toggle for each row uses a generic
aria-label causing screen readers to be unable to tell which file’s menu is
opening; update the ActionsColumn/actionsToggle rendering so the MenuToggle gets
a file-specific accessible name (e.g., use the row's file name or id from the
row data passed into actionsToggle) — for example compute an aria-label like
"Actions for <fileName>" or "More actions for <fileName>" using the row's file
identifier (from the same props/context that render the row) and set that string
on the MenuToggle aria-label so each row has a unique, descriptive label.
- Around line 442-445: The current onClick handler always calls onSelect(null,
true), making row clicks one-way and allowing nested interactive elements to
inadvertently trigger selection; update the onClick on the row wrapper (where
isUnselectable and onSelect are used) to (1) derive the new selection state from
the existing isSelected (e.g., toggle: nextSelected = !isSelected) and pass that
to onSelect instead of hardcoding true, and (2) ignore clicks originating from
interactive descendants by checking the event target (e.g., skip handling if the
click came from a button/input/a/checkbox or if event.defaultPrevented or
event.currentTarget !== event.target/using closest to detect interactive
elements) so folder-name clicks and inner controls do not bubble into select +
onViewDetails; apply this logic where folder-name click handling and the row
onClick are implemented to prevent double actions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 692cae06-5c50-468a-a1dc-a5127d900c0c
📒 Files selected for processing (2)
packages/automl/frontend/src/app/components/common/FileExplorer/FileExplorer.tsxpackages/autorag/frontend/src/app/components/common/FileExplorer/FileExplorer.tsx
| onClick={() => { | ||
| if (!isUnselectable) { | ||
| onSelect(null, true); | ||
| } |
There was a problem hiding this comment.
Guard row clicks from nested controls and toggle state.
Line 444 always passes true. That makes row clicks one-way in checkbox mode, and a folder-name click in Lines 475-478 also bubbles into a select + onViewDetails. Derive the next state from isSelected and ignore interactive descendants.
Patch
- onClick={() => {
- if (!isUnselectable) {
- onSelect(null, true);
- }
- }}
+ onClick={(event) => {
+ if (
+ isUnselectable ||
+ (event.target instanceof HTMLElement &&
+ event.target.closest('a, button, input, label'))
+ ) {
+ return;
+ }
+ onSelect(event, selection === 'checkbox' ? !isSelected : true);
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onClick={() => { | |
| if (!isUnselectable) { | |
| onSelect(null, true); | |
| } | |
| onClick={(event) => { | |
| if ( | |
| isUnselectable || | |
| (event.target instanceof HTMLElement && | |
| event.target.closest('a, button, input, label')) | |
| ) { | |
| return; | |
| } | |
| onSelect(event, selection === 'checkbox' ? !isSelected : true); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/autorag/frontend/src/app/components/common/FileExplorer/FileExplorer.tsx`
around lines 442 - 445, The current onClick handler always calls onSelect(null,
true), making row clicks one-way and allowing nested interactive elements to
inadvertently trigger selection; update the onClick on the row wrapper (where
isUnselectable and onSelect are used) to (1) derive the new selection state from
the existing isSelected (e.g., toggle: nextSelected = !isSelected) and pass that
to onSelect instead of hardcoding true, and (2) ignore clicks originating from
interactive descendants by checking the event target (e.g., skip handling if the
click came from a button/input/a/checkbox or if event.defaultPrevented or
event.currentTarget !== event.target/using closest to detect interactive
elements) so folder-name clicks and inner controls do not bubble into select +
onViewDetails; apply this logic where folder-name click handling and the row
onClick are implemented to prevent double actions.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/automl/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx (1)
250-260: Replace CSS-class assertion with behavior-based test.Line 259 couples the test to
pf-m-clickable, a PatternFly internal class name. This violates the guideline to "Follow PatternFly v6 patterns consistent with the main frontend app" — internal class contracts break on library updates.Assert the actual behavior instead: once selected, clicking the row should not trigger another selection callback.
Proposed test adjustment
- it('should make selected rows not clickable or selectable', () => { + it('should not re-trigger selection when clicking an already selected row', () => { const files = mockFiles(3); - render(<FileExplorer {...defaultProps} files={files} selection="checkbox" />); + const onSelectFile = jest.fn(); + render( + <FileExplorer + {...defaultProps} + files={files} + selection="checkbox" + onSelectFile={onSelectFile} + />, + ); const row = screen.getByTestId('file-explorer-row--file-1-json'); const checkbox = within(row).getByRole('checkbox'); fireEvent.click(checkbox); - // Row should not have clickable styling after selection - expect(row).not.toHaveClass('pf-m-clickable'); + expect(onSelectFile).toHaveBeenCalledTimes(1); + fireEvent.click(row); + expect(onSelectFile).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx` around lines 250 - 260, The test currently asserts a PatternFly internal class ('pf-m-clickable') on the row; instead, make it behavior-based: pass a mock selection callback in defaultProps (e.g., mockOnSelectionChange) when rendering <FileExplorer selection="checkbox" files={files} />, select the file by clicking the checkbox (fireEvent.click(checkbox)), then click the row element with testId 'file-explorer-row--file-1-json' (fireEvent.click(row)) and assert the mock selection callback was not invoked again (e.g., expect(mockOnSelectionChange).toHaveBeenCalledTimes(1) or not.toHaveBeenCalledTimes(2)), ensuring clicks on a selected row do not trigger another selection callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/autorag/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx`:
- Around line 250-260: The test is asserting an internal PatternFly CSS class
('pf-m-clickable') which is brittle; instead, mock the FileExplorer's
onSelectFile callback and assert whether it's invoked when clicking a row that's
already selected. Update the spec to spy/mock the onSelectFile prop passed into
FileExplorer (reference prop name onSelectFile) and replace the
expect(row).not.toHaveClass('pf-m-clickable') assertion with a behavioral check
that clicking the already-selected row either does not call onSelectFile (if row
onClick should be guarded) or does call it with the expected args (match current
intended behavior of the row onClick handler). Also, if the implementation lacks
guarding logic in the row onClick handler, consider adding a guard in the
component where the click handler checks selection state before invoking
onSelectFile (reference the row onClick handler).
---
Nitpick comments:
In
`@packages/automl/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx`:
- Around line 250-260: The test currently asserts a PatternFly internal class
('pf-m-clickable') on the row; instead, make it behavior-based: pass a mock
selection callback in defaultProps (e.g., mockOnSelectionChange) when rendering
<FileExplorer selection="checkbox" files={files} />, select the file by clicking
the checkbox (fireEvent.click(checkbox)), then click the row element with testId
'file-explorer-row--file-1-json' (fireEvent.click(row)) and assert the mock
selection callback was not invoked again (e.g.,
expect(mockOnSelectionChange).toHaveBeenCalledTimes(1) or
not.toHaveBeenCalledTimes(2)), ensuring clicks on a selected row do not trigger
another selection callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 66d9f8d3-dd28-4196-86ed-6745b5ea2708
📒 Files selected for processing (2)
packages/automl/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsxpackages/autorag/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx
There was a problem hiding this comment.
Code Review
Generally the change looks good and works well in the FileExplorer.playground file.
Some things I noticed though
A11Y 🐛 - Keyboard navigation doesn't work as expected
When I tab through the elements, and the whole row is highlighted, neither Enter or Space result in the row being clicked as if I had clicked it myself
A11Y 🐛 - Not enough distinction between row click and directory click
The 'before' wasn't much better since the small directory pf-link has only a slightly darker hue to it on hover, but in this 'before' it was very clear since the row wasn't clickable at all.
Now that it is, I think we should have some better styling on the directory name. Maybe on hover/focus of the directory link it should become an underlined link?
UX 🧑💻 - Multi select click of a row does not deselect it
Although this is not a feature made available in AutoX - The FileExplorer still exposes this capability.
When I click on a checkbox on/off it correctly adds/removes the selected file/folder.
But when I click a row multiple times it only stays as selected.
UX 🧑💻 - Should FileExplorer make this an optional prop?
General question for future use: Should FileExplorer expose this as a prop that consumers give us or should we always make rows clickable?
…into feat/autox-file-explorer-row-click
|
@GAUNSD thanks for reviewing and bringing up these great points i pushed some fixes so keyboard row clicks should work now, the checkbox behaviour should also be fixed, and the folder links are now using buttons to be more semantically correct but also add the underline style as you were suggesting however, the underline style is persistent instead of appearing on hover/click, but in my opinion i think this is okay as it is consistent with the link buttons that we have in the breadcrumbs as for whether it should be an optional prop, i can't think of any scenario where you would want the rows to be non-clickable, i think its better for QoL and accessibility as it makes the rows easier to click instead of having to click the tiny radio/checkbox and we have fewer edge cases with additional logic heres a video showing the new changes! Screen.Recording.2026-04-14.at.11.48.52.AM.mov |
|
@GAUNSD two things i did noticed while i was testing/recording though was that the preview eye icon disappears after selection in checkbox mode do you have fixes or are addressing any of these in any other PRs, if not I can also add fixes for those here |
…into feat/autox-file-explorer-row-click
|
Thanks for the changes @daniduong!
I did not have fixes for the points you raised so feel free to fix in this PR. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/automl/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx`:
- Around line 598-600: The test currently fires fireEvent.keyDown(folderLink, {
key: 'Enter' }) and then immediately fireEvent.click(folderLink), which lets the
mouse click satisfy onFolderClick even if the Enter handler is broken; split
keyboard and mouse assertions by first firing fireEvent.keyDown(folderLink, {
key: 'Enter', code: 'Enter' }) and asserting the mock onFolderClick (or relevant
callback) was called the expected number of times, then in a separate step fire
fireEvent.click(folderLink) and assert the call count incremented accordingly
(use the mock function referenced as onFolderClick and the DOM node folderLink
to locate the code to change), or simply remove the click to test only the Enter
behavior.
- Around line 387-406: The final assertion in the "should navigate between radio
inputs using arrow keys" test is ineffective; after firing ArrowDown it only
checks radio2 exists. Replace that check with an observable post-condition using
the existing radio1/radio2 references: e.g., after fireEvent.keyDown(radio1, {
key: 'ArrowDown' }), assert that radio2 has focus (expect(radio2).toHaveFocus()
or expect(document.activeElement).toBe(radio2)) or that radio2 became checked
(expect(radio2).toBeChecked()) depending on the intended behavior, so the test
actually fails if arrow-key navigation regresses.
In
`@packages/autorag/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx`:
- Around line 583-607: The test is a false positive because firing keyDown on
the folder link has no effect (the link is rendered as a Button with only an
onClick), so remove the keyDown from the click test and split into two focused
tests: 1) Update the existing test around FileExplorer in FileExplorer.spec.tsx
(the test using getByTestId('file-explorer-row--my-folder') and onFolderClick)
to only simulate the click (fireEvent.click) and assert onFolderClick was called
and the checkbox remains unchecked; 2) Add a new isolated keyboard behavior test
that specifically asserts keyboard Enter behavior for the folder link (use the
same row/folderLink lookup and fireEvent.keyDown(folderLink, { key: 'Enter',
code: 'Enter' }) and assert whether onFolderClick is called or not, matching the
intended behavior) so keyboard activation is verified independently of click
handling.
- Around line 387-407: The final assertion is unreachable because radio2 is
queried before the ArrowDown event; instead, update the test "should navigate
between radio inputs using arrow keys" to assert that the selection callback
passed in defaultProps was not invoked by ArrowDown: make sure the selection
handler in defaultProps is a jest.fn (e.g., defaultProps.onSelect or
defaultProps.onSelectionChange), fire the ArrowDown key on radio1 as you already
do, and then assert
expect(defaultProps.<selectionHandler>).not.toHaveBeenCalled(); leave the Space
key behavior assertion unchanged (radio1 checked) to confirm selection still
works with Space.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a3db2bd-efcd-4232-9808-4802a1dfb29e
📒 Files selected for processing (4)
packages/automl/frontend/src/app/components/common/FileExplorer/FileExplorer.tsxpackages/automl/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsxpackages/autorag/frontend/src/app/components/common/FileExplorer/FileExplorer.tsxpackages/autorag/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/autorag/frontend/src/app/components/common/FileExplorer/FileExplorer.tsx
- packages/automl/frontend/src/app/components/common/FileExplorer/FileExplorer.tsx
Human code review 👤 🔍Tested this manually and all changes raised earlier and UX is perfect. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7114 +/- ##
==========================================
- Coverage 64.81% 63.64% -1.17%
==========================================
Files 2441 2496 +55
Lines 75996 77538 +1542
Branches 19158 19703 +545
==========================================
+ Hits 49254 49351 +97
- Misses 26742 28187 +1445 see 61 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GAUNSD The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 975bac0
into
opendatahub-io:main
opendatahub-io#7114) * feat(autox): support selecting files by clicking rows in file explorer * fix(automl+autorag): guard row clicks from nested controls * test(automl+autorag): add tests to validate clicking rows * fix: allow toggle on click in checkbox mode * test: update/add tests to validate checkbox toggle on row click * fix: support keyboard row clicks by addressing typo * test: add tests to validate keyboard row clicks * fix: use inline "link" btn over href="#" txt (semantics and styling) * fix: simplify `isSelectable` and `isClickable` logic for rows * fix: handle kb evts failing when using `onRowClick` + radio/checkbox * feat: display eye for selected file in details if multiple selected * test: address coderabbit --------- Co-authored-by: Daniel Duong <danielduong@ibm.com>
2 participants
https://redhat.atlassian.net/browse/RHOAIENG-58659
Description
This PR enhances the FileExplorer component (used in AutoML and AutoRAG) to support row-click selection, improving the user experience for file and folder navigation.
Key Changes
Unified Row Selection Behavior:
Enhanced Folder Navigation:
Visual Feedback:
Keyboard & Accessibility Improvements:
onRowClickwith radio/checkbox selectionTest Coverage
Added comprehensive test coverage including:
How Has This Been Tested?
Test Impact
FileExplorer.spec.tsxin both automl and autorag packagesRequest review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit