UI: Improve dataset upload loading & feedback #82
Conversation
Summary of ChangesHello @sahilmane69, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for uploading training datasets by introducing a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new DatasetUpload component with improved user experience for file uploads, including loading states and feedback messages. The changes are well-structured. My review focuses on improving maintainability, accessibility, and robustness of the new component. I've suggested adding client-side file validation, improving error handling, making the component more accessible for screen reader users, and extracting hardcoded values into constants for better maintainability.
| const handleFileChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (e.target.files && e.target.files[0]) { | ||
| setFile(e.target.files[0]) | ||
| setStatus(null) | ||
| } | ||
| } |
There was a problem hiding this comment.
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., text/csv) and potentially the file size to provide immediate feedback to the user and prevent unnecessary uploads of invalid files. This improves user experience and reduces load on the backend.
const handleFileChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const selectedFile = e.target.files?.[0];
if (selectedFile) {
if (!selectedFile.type.includes('csv')) {
setStatus({ type: 'error', message: 'Invalid file type. Please upload a CSV file.' });
setFile(null);
e.target.value = ''; // Reset input to allow re-selecting the same file
return;
}
setFile(selectedFile);
setStatus(null);
}
}
| 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) |
There was a problem hiding this comment.
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:
const STATUS_MESSAGES = {
SUCCESS: 'Dataset uploaded successfully!',
FAILURE: 'Failed to upload dataset. Please try again.',
NETWORK_ERROR: 'Network error. Please check your connection.',
};
// Then use it like:
setStatus({ type: 'success', message: STATUS_MESSAGES.SUCCESS });| formData.append('file', file) | ||
|
|
||
| try { | ||
| const response = await fetch('/api/v1/training-datasets/upload', { |
There was a problem hiding this comment.
The API endpoint URL /api/v1/training-datasets/upload is hardcoded. It's better to store it in a central configuration file or as a constant. This makes it easier to manage and update API endpoints, especially if they are used in multiple places or need to change between environments.
For example, you could define a constant at the top of the component:
const UPLOAD_API_ENDPOINT = '/api/v1/training-datasets/upload';
...and then use it in the fetch call.
| } else { | ||
| setStatus({ type: 'error', message: 'Failed to upload dataset. Please try again.' }) | ||
| } |
There was a problem hiding this comment.
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.
} else {
let errorMessage = 'Failed to upload dataset. Please try again.';
try {
const errorData = await response.json();
if (errorData && errorData.message) {
errorMessage = errorData.message;
}
} catch {
// Ignore if response is not JSON
}
setStatus({ type: 'error', message: errorMessage });
}
| <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" | ||
| /> |
There was a problem hiding this comment.
This file input is missing an associated <label>. Labels are crucial for accessibility as they provide context for screen reader users. You should wrap the input with a label. You can add a visually hidden span inside the label for screen reader text (e.g., using Tailwind's sr-only class).
<label>
<span className="sr-only">Choose a file to upload</span>
<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"
/>
</label>
| <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> |
There was a problem hiding this comment.
The status message updates dynamically, but screen readers may not announce these changes. To improve accessibility, you should add role="status" and aria-live="polite" to the status message container. This ensures that users of assistive technologies are notified of the upload status.
<div
role="status"
aria-live="polite"
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>
|
@sahilmane69 Please Refer Figma file to make Ui Batter |
2 participants
This PR improves the UX of the existing DatasetUpload component by:
No backend or API changes.
Related to Issue #59.