fix(autox): fix 500 error after many duplicate file uploads#7220
Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThis pull request adds a sentinel error Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Actionable Issues
Apply fixes above to ensure safe, robust client behavior when rendering and interpreting BFF error payloads. 🚥 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 review |
✅ Actions performedReview triggered.
|
daniduong
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/automl/frontend/src/app/components/configure/AutomlConfigure.tsx (1)
345-352: Do not branch the upload UX on an English error substring.
includes('unique filename')couples this flow to one backend message. Any wording change or localization will silently drop the 409-specific guidance even though the API behavior is unchanged. Have the upload mutation expose a stable discriminator such as HTTP status or backend error code, and branch on that here instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/frontend/src/app/components/configure/AutomlConfigure.tsx` around lines 345 - 352, The current upload error handling in AutomlConfigure.tsx branches on the English substring 'unique filename' (in the notification.error call) which is brittle; instead change the upload mutation to return/throw a stable discriminator (HTTP status or backend error code) and update the catch logic in the upload handler to detect that discriminator (e.g., err.response?.status === 409 or err.code === 'FILENAME_CONFLICT') and only then show the specific 409 guidance; otherwise show the generic errorMessage. Update references in AutomlConfigure to use the mutation's stable error field and change the isConflict check to use that field (not string.includes).
🤖 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/hooks/mutations.ts`:
- Around line 112-120: The error handling collapses the BFF envelope into
Error(message) and trusts errorResponse?.error?.message is a string; change the
logic in the upload error branch that parses xhr.responseText so you validate
the parsed payload and preserve status/code: parse JSON into errorResponse,
confirm types (e.g., typeof errorResponse?.error?.message === "string"), then
construct and reject a richer Error that includes both a clear message and
preserves metadata (attach status/xhr.status and the parsed body or an errorCode
field to the thrown object or a custom Error instance) instead of only
reject(new Error(errorMessage)); ensure callers can inspect the status/code and
original parsed envelope.
---
Nitpick comments:
In `@packages/automl/frontend/src/app/components/configure/AutomlConfigure.tsx`:
- Around line 345-352: The current upload error handling in AutomlConfigure.tsx
branches on the English substring 'unique filename' (in the notification.error
call) which is brittle; instead change the upload mutation to return/throw a
stable discriminator (HTTP status or backend error code) and update the catch
logic in the upload handler to detect that discriminator (e.g.,
err.response?.status === 409 or err.code === 'FILENAME_CONFLICT') and only then
show the specific 409 guidance; otherwise show the generic errorMessage. Update
references in AutomlConfigure to use the mutation's stable error field and
change the isConflict check to use that field (not string.includes).
🪄 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: 32e0f757-c916-443e-8b71-821046e59725
📒 Files selected for processing (11)
packages/automl/bff/internal/api/s3_handler.gopackages/automl/bff/internal/api/s3_handler_test.gopackages/automl/frontend/src/app/components/configure/AutomlConfigure.tsxpackages/automl/frontend/src/app/components/configure/__tests__/AutomlConfigure.spec.tsxpackages/autorag/bff/internal/api/s3_handler.gopackages/autorag/bff/internal/api/s3_handler_test.gopackages/autorag/frontend/src/app/components/configure/AutoragConfigure.tsxpackages/autorag/frontend/src/app/components/configure/AutoragEvaluationSelect.tsxpackages/autorag/frontend/src/app/components/configure/__tests__/AutoragConfigure.spec.tsxpackages/autorag/frontend/src/app/components/configure/__tests__/AutoragEvaluationSelect.spec.tsxpackages/autorag/frontend/src/app/hooks/mutations.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7220 +/- ##
==========================================
- Coverage 64.82% 64.80% -0.02%
==========================================
Files 2441 2441
Lines 76004 75996 -8
Branches 19159 19158 -1
==========================================
- Hits 49266 49248 -18
- Misses 26738 26748 +10 see 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
MatthewAThompson
left a comment
MatthewAThompson
left a comment
There was a problem hiding this comment.
Thanks @daniduong
AI-assisted review:
MUST FIX (Blocking):
Use HTTP status code (409) instead of string matching for error detection
- More robust and follows HTTP semantics
- Prevents breakage if message wording changes
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/api/s3.ts`:
- Around line 99-107: The code calls response.json() unconditionally which can
throw on empty/malformed/HTML error bodies and prevent attaching statusCode to
the thrown Error; wrap the JSON parse in a try/catch (or attempt response.text()
fallback) so you safely obtain responseData (or rawBody) even when parsing
fails, then when !response.ok always construct and throw an Error that includes
the descriptive message (using responseData?.error?.message or the raw text) and
attach { statusCode: response.status } to that Error (the existing variable
names response, responseData, and the created error object should be reused so
callers relying on error.statusCode can still detect 409 conflicts).
- Around line 94-97: uploadFileToS3 currently does a direct fetch POST which
bypasses the app's restCREATE()/handleRestFailures() abstraction and omits
explicit credentials and robust error parsing; change uploadFileToS3 to use
restCREATE() (and handleRestFailures()) for consistency if it supports FormData,
otherwise keep fetch but add credentials: 'same-origin', check response.ok
before calling response.json(), and if not ok read text/json conditionally to
attach statusCode to the thrown error; reference uploadFileToS3, restCREATE, and
handleRestFailures when making the change.
In `@packages/autorag/frontend/src/app/api/s3.ts`:
- Around line 99-108: The response.json() call is done before checking
response.ok which can throw and prevent attaching statusCode; change the flow to
first check if (!response.ok) then attempt to parse the body inside a try/catch
(e.g., call response.json() or response.text() as fallback) so you can always
construct an Error with a message and attach { statusCode: response.status }
before throwing; ensure the code paths that currently use responseData,
errorMessage and the thrown error still work when JSON parsing fails by using
fallback text or a default message.
🪄 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: c9ff5f3d-fe96-4e1a-adb0-9feb09b969cc
📒 Files selected for processing (4)
packages/automl/frontend/src/app/api/s3.tspackages/automl/frontend/src/app/components/configure/AutomlConfigure.tsxpackages/autorag/frontend/src/app/api/s3.tspackages/autorag/frontend/src/app/components/configure/AutoragConfigure.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/automl/frontend/src/app/components/configure/AutomlConfigure.tsx
|
@MatthewAThompson if it's fine with you i think i might just revert to the previous commit where coderabbit wasn't complaining as much 😅, i don't think the string matching is truly blocking tbh, we have that pattern in a couple places already, we can probably address all these later as tech debt |
This reverts commit bc3b631.
|
@daniduong yeah, I think it's ok to revert back to string matching. We can address later. |
|
/lgtm |
|
Automated tests are clean: 
AI-assisted review found no actionable issues. Manual testing looks good: 
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrjones-rh 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 e273529
into
opendatahub-io:main
3 participants
https://redhat.atlassian.net/browse/RHOAIENG-58660
Description
This PR addresses error handling improvements in the S3 file upload collision resolution logic for AutoML and AutoRAG packages.
Changes Made
Backend (AutoML & AutoRAG BFFs):
ErrMaxCollisionsExceededsentinel error for proper error classificationFrontend (AutoRAG):
useUploadToStorageMutation- now correctly extracts BFF error messages from JSON responseWhy This Matters
Previously, when the BFF exhausted all attempts (default 10) to find a unique filename due to naming collisions (e.g.,
file.pdf,file-1.pdf, ...file-10.pdfall exist), it would:"Upload failed with status 500"Now:
Future Consideration
A modal dialog to let users select a different filename when a collision occurs will be considered for future enhancement.
How Has This Been Tested?
Unit Tests
TestPostS3FileHandler_CollisionResolutionExhausted_Returns409for both AutoML and AutoRAGManual Testing
Screen.Recording.2026-04-14.at.10.35.09.AM.mov
Screen.Recording.2026-04-14.at.10.30.59.AM.mov
Test Impact
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
Release Notes
Bug Fixes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests