fix(autorag): fix s3 button not opening file explorer#7213
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThree files updated: documentation for AutoML pipeline name prefix defaults, FileSelector component prop placement adjustment, and test assertions for AutoragEvaluationSelect. The AutoML documentation changed time-series and tabular pipeline name prefix defaults from shorter to longer namespace variants. The FileSelector component moved the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/autorag/frontend/src/app/components/common/FileSelector.tsx (1)
59-60: ComposeonClearClickinstead of relying on prop override order.Lines 59–60 depend on prop ordering:
fileUploadProps.onClearClickreplaces the local handler when provided, skippingsetUploadedFile(undefined). PatternFly v6 FileUpload component recommends composing handlers—invoke local logic and then the consumer handler (if provided). This pattern ensures proper state cleanup while supporting custom actions.Proposed change
<FileUpload className="pf-v6-u-mt-sm" - onClearClick={() => setUploadedFile(undefined)} - {...props.fileUploadProps} + {...props.fileUploadProps} + onClearClick={(event) => { + setUploadedFile(undefined); + props.fileUploadProps?.onClearClick?.(event); + }} id={props.id}🤖 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/FileSelector.tsx` around lines 59 - 60, The current FileSelector uses onClearClick={() => setUploadedFile(undefined)} and spreads {...props.fileUploadProps} which allows fileUploadProps.onClearClick to override the local cleanup; instead compose the handler so setUploadedFile(undefined) always runs then call the consumer callback if provided: create a composedOnClearClick that calls setUploadedFile(undefined) and then, if props.fileUploadProps?.onClearClick exists, invokes it with the same arguments (preserving event/args), and pass that composedOnClearClick along with the rest of props.fileUploadProps when rendering the FileUpload component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/autorag/frontend/src/app/components/common/FileSelector.tsx`:
- Around line 59-60: The current FileSelector uses onClearClick={() =>
setUploadedFile(undefined)} and spreads {...props.fileUploadProps} which allows
fileUploadProps.onClearClick to override the local cleanup; instead compose the
handler so setUploadedFile(undefined) always runs then call the consumer
callback if provided: create a composedOnClearClick that calls
setUploadedFile(undefined) and then, if props.fileUploadProps?.onClearClick
exists, invokes it with the same arguments (preserving event/args), and pass
that composedOnClearClick along with the rest of props.fileUploadProps when
rendering the FileUpload component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c15283cd-f29e-42e4-9faa-36ef7a52b230
📒 Files selected for processing (3)
packages/automl/docs/pipeline-runs-api.mdpackages/autorag/frontend/src/app/components/common/FileSelector.tsxpackages/autorag/frontend/src/app/components/configure/__tests__/AutoragEvaluationSelect.spec.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7213 +/- ##
==========================================
+ Coverage 64.79% 64.81% +0.02%
==========================================
Files 2447 2441 -6
Lines 76059 76004 -55
Branches 19172 19159 -13
==========================================
- Hits 49279 49259 -20
+ Misses 26780 26745 -35 see 19 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Works great thanks @daniduong AI Assisted reviewOverall AssessmentApprove — The core fix is correct, minimal, and well-explained. The test improvements are a net positive. A few minor suggestions below. Suggestions
Test ValidationautoragFrontend ✅BFF ✅Contract ✅Manual testingSelecting a file from s3 is working now. Browse from computer has not regressed |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Added a commit to address suggestions 1 and 2: Null guards added — All three container.querySelector calls now have expect(uploadInput).not.toBeNull() before the cast, so if the DOM structure ever changes, the test will fail with a clear "expected not to be null" message instead of a cryptic error. data-testid not feasible — PF's FileUpload renders the file input via react-dropzone's getInputProps() internally. There's no prop to pass a data-testid through, so container.querySelector is the correct approach here. |
|
/lgtm |
|
Automated testing looks good: 
AI-assisted review revealed no actionable items. Manual testing looks good: Thanks for the quick fix @daniduong. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrjones-rh, nickmazzi 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 f3eccea
into
opendatahub-io:main
3 participants
https://redhat.atlassian.net/browse/RHOAIENG-57763
Description
The Bug
The FileSelector component had a spread operator ordering bug that prevented
fileUploadProps.onClearClickfrom overriding the default clear behavior.In the FileUpload component,
onClearClickwas defined after the spread operator:This meant that the internal
onClearClickhandler always took precedence, preventing consumers from customizing the clear button behavior.Impact
In
AutoragEvaluationSelect, clicking the S3 button should open the S3 file explorer, but instead it was only clearing the internal upload state because the customonClearClickwas being overridden.The Fix
Moved
onClearClickbefore the spread operator, allowingfileUploadProps.onClearClickto override it:Test Improvements
The existing
AutoragEvaluationSelect.spec.tsxtests had a mock FileSelector that was too simplified and didn't catch this bug. The mock directly called the prop without testing the actual spread behavior.Changes:
getByRole,getByPlaceholderText) instead of test IDsThis makes the tests more realistic and catches prop ordering bugs like this one.
How Has This Been Tested?
Unit Tests:
AutoragEvaluationSelect.spec.tsxpass (including the critical "should open S3FileExplorer when S3 button is clicked" test)FileSelector.spec.tsxpassManual Testing:
Screen.Recording.2026-04-13.at.10.18.31.AM.mov
Test Impact
Modified:
packages/autorag/frontend/src/app/components/configure/__tests__/AutoragEvaluationSelect.spec.tsxNo changes needed:
packages/autorag/frontend/src/app/components/common/__tests__/FileSelector.spec.tsxModified:
packages/autorag/frontend/src/app/components/common/FileSelector.tsxonClearClickprop placement (non-breaking change, improves API)Request 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
Documentation
Refactor
Tests