TT-7315 handle upload busy state#316
Open
gtryus wants to merge 6 commits into
Open
Conversation
…oadsDialog components - Added onReadyToUpload prop to manage upload readiness without blocking. - Removed direct setBusy calls in PendingUploadsDialog to streamline busy state management. - Updated nextUpload action to invoke onReadyToUpload after import/export wait completes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an onReadyToUpload callback to the upload flow so UI/components can set “busy” state after any import/export-idle waiting completes, avoiding self-deadlocks when importexportBusy is used both as a gate and as an upload-busy flag.
Changes:
- Added
onReadyToUploadtonextUploadprops and invoked it afterwaitForImportExportIdle()and before staging/POST. - Updated
UploaderandPendingUploadsDialogto useonReadyToUploadinstead of setting busy state up-front. - Added Jest coverage for the new busy/wait ordering in
nextUpload.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/renderer/src/store/upload/actions.uploadBusy.test.ts | Adds tests to assert callback ordering and avoid deadlock scenarios. |
| src/renderer/src/store/upload/actions.tsx | Extends nextUpload API with onReadyToUpload and calls it after import/export idle wait. |
| src/renderer/src/components/Uploader.tsx | Switches busy-state setting to onReadyToUpload (removes immediate setBusy(true)). |
| src/renderer/src/components/Team/PendingUploadsDialog.tsx | Removes immediate dialog busy toggles and sets dialog busy via onReadyToUpload. |
- Reintroduced setBusy calls in handleRetryOne and handleRetryAll functions to ensure proper busy state management during upload retries. - Removed the onReadyToUpload callback to streamline the upload process.
- Introduced isFirstFile flag to determine when to wait for external import/export during uploads. - Updated getImportExportBusy and onReadyToUpload callbacks to only activate for the first file in a batch, improving upload efficiency. - Added test case to verify behavior when getImportExportBusy is omitted for subsequent files.
gtryus
changed the title
Comment on lines
406
to
412
| void (async () => { | ||
| if (getImportExportBusy) { | ||
| await waitForImportExportIdle(getImportExportBusy); | ||
| } | ||
| onReadyToUpload?.(); | ||
|
|
||
| let localAbsolutePath = ''; |
| /* eslint-disable @typescript-eslint/no-require-imports */ | ||
| import Axios from 'axios'; | ||
| import { UploadType } from '../../components/UploadType'; | ||
| import { MediaFileAttributes } from '../../model'; |
added 2 commits
May 19, 2026 16:44
- Modified nextUpload action to call onReadyToUpload when offline, ensuring proper upload readiness before local write. - Enhanced documentation for onReadyToUpload to clarify its usage in both offline and online scenarios. - Added a test case to verify the invocation of onReadyToUpload in the offline path.
- Introduced a new test case to verify that onReadyToUpload is not invoked when offline validation fails. - Enhanced existing test to ensure onReadyToUpload is called before local write during successful offline uploads. - Updated the order of operations in the tests to reflect the expected sequence of events.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/renderer/src/store/upload/actions.tsx:283
- In the offline path,
writeFileLocal(...).then(...)is wrapped in atry/catch, but because the promise is not awaited and there is no.catch, any rejection fromwriteFileLocalwill bypass this handler. That can leave the upload flow without callingcb/sendError, potentially hanging the batch (and leavingimportexportBusystuck true whenonReadyToUploadsets it). Handle promise rejections explicitly (e.g.,awaitinsidetry/catchor add a.catchthat callssendError).
if (offline) {
onReadyToUpload?.();
if (!isDownloadable) {
if (cb) cb(n, true, { ...record });
} else
try {
writeFileLocal(files[n] as File).then((w) => {
if (cb) cb(n, true, { ...record, audioUrl: w.relativeMediaPath });
});
} catch (err: unknown) {
Comment on lines
+110
to
+111
| // uploadFile uses XHR; skip PUT by using text/plain non-downloadable - no, mp3 is downloadable | ||
| // Mock uploadFile path via successful PUT - need to mock XMLHttpRequest |
- Introduced importWasBusyRef to determine if import/export was busy at the start of the current batch. - Updated getImportExportBusy logic to account for the busy state of previous imports, improving upload handling for subsequent files. - Ensured busy state is set correctly based on the import/export status when starting uploads.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.