-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add reset button to footer #24
base: main
Are you sure you want to change the base?
Conversation
It's good practice to include screenshots of UI changes so reviewers can understand the UI impact without having to pull the branch and spin up the app, can you do so here, please? |
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.
lgtm, one minor question about whether we should show the error message if reset fails
setModels([]); | ||
setTranscriptions([]); | ||
} else { | ||
console.error('Error resetting app'); |
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.
Would it be worth showing the error message to help diagnose if the reset failed?
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.
I can make this an alert if you'd prefer for all users to see this (which I don't think any will be still). For context, the only way the request is going to fail here should be if the server container is down. The server container also get's restarted immediately if it ever goes down.
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.
Ah, that's fine then. No need to propagate that error.
Pressing Reset during training creates havoc. I suspect that this is triggered by the reset process deleting the model directory, causing the log process to fail due to missing file (can't open the file), causing the reset to fail. This has flow-on effect such as the data and the interface getting out of sync due to dataset data being deleted but the dataset.json file remaining. Could be fixed by monitoring data-processing, training and transcription processes, but that sounds complicated. May be able to catch 500 status due to missing files and update the GUI accordingly? More detailed error message:
|
Closes #16