-
Notifications
You must be signed in to change notification settings - Fork 45
UI: Improve dataset upload loading & feedback #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| node_modules/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import React, { useState } from 'react' | ||
|
|
||
| const DatasetUpload: React.FC = () => { | ||
| const [file, setFile] = useState<File | null>(null) | ||
| const [isUploading, setIsUploading] = useState(false) | ||
| const [status, setStatus] = useState<{ type: 'success' | 'error'; message: string } | null>(null) | ||
|
|
||
| const handleFileChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (e.target.files && e.target.files[0]) { | ||
| setFile(e.target.files[0]) | ||
| setStatus(null) | ||
| } | ||
| } | ||
|
Comment on lines
+8
to
+13
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The component currently doesn't perform any client-side validation on the selected file. Based on the backend code, it seems to expect a CSV file. You should add validation to check the file type (e.g., |
||
|
|
||
| const handleSubmit = async () => { | ||
| if (!file) return | ||
|
|
||
| if (isUploading) return | ||
|
|
||
| setIsUploading(true) | ||
| setStatus(null) | ||
|
|
||
| const formData = new FormData() | ||
| formData.append('file', file) | ||
|
|
||
| try { | ||
| const response = await fetch('/api/v1/training-datasets/upload', { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API endpoint URL For example, you could define a constant at the top of the component: |
||
| method: 'POST', | ||
| body: formData, | ||
| }) | ||
|
|
||
| if (response.ok) { | ||
| setStatus({ type: 'success', message: 'Dataset uploaded successfully!' }) | ||
| setFile(null) | ||
| } else { | ||
| setStatus({ type: 'error', message: 'Failed to upload dataset. Please try again.' }) | ||
| } | ||
|
Comment on lines
+35
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error message for a failed upload is generic. The backend might provide a more specific error message in the response body. You should attempt to parse the response to get a more descriptive error message. This will improve the user experience by providing more context about what went wrong. |
||
| } catch (error) { | ||
| setStatus({ type: 'error', message: 'Network error. Please check your connection.' }) | ||
| } finally { | ||
| setIsUploading(false) | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <div className="p-6 bg-gray-900 rounded-lg border border-gray-800 text-white max-w-lg mx-auto mt-10"> | ||
| <h2 className="text-xl font-semibold mb-4">Upload Training Data</h2> | ||
|
|
||
| <div className="flex flex-col gap-4"> | ||
| <input | ||
| type="file" | ||
| onChange={handleFileChange} | ||
| disabled={isUploading} | ||
| className="block w-full text-sm text-gray-400 | ||
| file:mr-4 file:py-2 file:px-4 | ||
| file:rounded-full file:border-0 | ||
| file:text-sm file:font-semibold | ||
| file:bg-indigo-600 file:text-white | ||
| hover:file:bg-indigo-700 | ||
| disabled:opacity-50 disabled:cursor-not-allowed" | ||
| /> | ||
|
Comment on lines
+50
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file input is missing an associated |
||
|
|
||
| {status && ( | ||
| <div | ||
| className={`p-3 rounded text-sm ${ | ||
| status.type === 'success' | ||
| ? 'bg-green-900/50 text-green-300' | ||
| : 'bg-red-900/50 text-red-300' | ||
| }`} | ||
| > | ||
| {status.message} | ||
| </div> | ||
|
Comment on lines
+64
to
+72
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The status message updates dynamically, but screen readers may not announce these changes. To improve accessibility, you should add |
||
| )} | ||
|
|
||
| <button | ||
| onClick={handleSubmit} | ||
| disabled={!file || isUploading} | ||
| className={`px-4 py-2 rounded font-medium transition-colors | ||
| ${ | ||
| !file || isUploading | ||
| ? 'bg-gray-700 text-gray-400 cursor-not-allowed' | ||
| : 'bg-indigo-600 text-white hover:bg-indigo-700' | ||
| } | ||
| `} | ||
| > | ||
| {isUploading ? 'Uploading...' : 'Submit Dataset'} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| export default DatasetUpload | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component uses hardcoded strings for status messages on lines 33, 36, and 39. It's a best practice to extract these into a constants object. This improves maintainability, reduces the chance of typos, and makes future internationalization easier.
Example: