Fixed: Re-newed import/export request failed immediately if the previous failed #8649
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request includes several updates aimed at fixing errors and enhancing functionality within the application. Key changes involve resolving specific error handling issues, restructuring import and export functionalities, and modifying the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
cvat-core/src/requests-manager.ts (1)
135-138: Consider adding error loggingWhile the error handling is correct, consider adding error logging to help with debugging production issues.
} + console.error(`Request ${requestID} failed:`, error); delete this.listening[requestID]; reject(error);cvat-ui/src/actions/export-actions.ts (1)
28-29: Consider moving thecoreinitialization to a centralized location.Initializing the
coreobject directly in this file may lead to inconsistencies if it is being initialized in multiple places. Consider moving the initialization to a centralized location and importing it where needed.cvat-ui/src/actions/import-actions.ts (6)
72-74: Fix JSDoc comment formattingThe JSDoc comment has an extra asterisk, which may cause formatting issues with documentation tools.
Apply this diff to correct the comment:
-/** * +/** * Function is supposed to be used when a new dataset import request initiated by a user -** */ + */
87-90: Use the existinglistenhelper functionInstead of defining a local
listenForImportfunction, consider using thelistenhelper function imported from./requests-actionsfor consistency and to reduce redundancy.Apply this diff to use the
listenfunction:- const listenForImport = (rqID: string) => core.requests.listen(rqID, { - callback: (updatedRequest) => updateRequestProgress(updatedRequest, dispatch), - }); - ... - await listenForImport(rqID); + await listen(rqID, dispatch);
163-165: Fix JSDoc comment formattingThe JSDoc comment for
importBackupAsynchas an extra asterisk at the beginning and end.Apply this diff to correct the comment:
-/** * +/** * Function is supposed to be used when a new backup import request initiated by a user -** */ + */
171-174: Use thelistenhelper function for consistencyIn
importBackupAsync, you're usingcore.requests.listendirectly. For consistency with other parts of the code, consider using thelistenhelper function.Apply this diff to use the
listenfunction:- const result = await core.requests.listen(rqID, { - callback: (updatedRequest) => updateRequestProgress(updatedRequest, dispatch), - }); + const result = await listen(rqID, dispatch);
182-184: Fix JSDoc comment formattingThe JSDoc comment for
listenImportBackupAsynccontains extra asterisks.Apply this diff to correct the comment:
-/** * +/** * Function is supposed to be used when application starts listening to existing backup import request -** */ + */
201-203: Fix JSDoc comment formattingThe JSDoc comment for
listenImportDatasetAsynchas formatting issues due to extra asterisks.Apply this diff to correct the comment:
-/** * +/** * Function is supposed to be used when application starts listening to existing dataset import request -** */ + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
changelog.d/20241106_095739_sekachev.bs_fixed_exception_1.md(1 hunks)changelog.d/20241106_112907_sekachev.bs_fixed_exception_2.md(1 hunks)cvat-core/src/requests-manager.ts(3 hunks)cvat-ui/src/actions/export-actions.ts(4 hunks)cvat-ui/src/actions/import-actions.ts(5 hunks)cvat-ui/src/actions/requests-actions.ts(1 hunks)cvat-ui/src/cvat-core-wrapper.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- changelog.d/20241106_095739_sekachev.bs_fixed_exception_1.md
- cvat-ui/src/actions/requests-actions.ts
🔇 Additional comments (11)
cvat-ui/src/cvat-core-wrapper.ts (2)
31-31: LGTM! Clean addition of StorageLocation enum import
The StorageLocation enum is properly imported alongside related enums, maintaining consistent code organization.
112-112: LGTM! Verify StorageLocation enum usage
The StorageLocation enum is properly exported. Let's verify its usage across the codebase to ensure it's properly integrated with the export functionality.
✅ Verification successful
Based on the search results, I can now generate the final response since I have comprehensive information about the StorageLocation enum usage across the codebase:
StorageLocation enum is properly integrated with export functionality
The StorageLocation enum is correctly used throughout the codebase for handling storage locations in:
- Core functionality (cvat-core)
- UI components for export/import operations
- Project/task creation flows
- Backup/restore operations
The enum is consistently used with two values (LOCAL and CLOUD_STORAGE) and properly integrated with:
- Storage class implementation
- Server communication layer
- UI storage selection components
- Export/backup workflows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of StorageLocation enum in the codebase
# Expected: Find usage in export-related functionality, particularly with targetStorage parameter
# Search for StorageLocation usage
echo "=== StorageLocation Usage ==="
rg "StorageLocation" -A 3
# Search for targetStorage parameter usage
echo -e "\n=== targetStorage Parameter Usage ==="
rg "targetStorage" -A 3
Length of output: 97767
cvat-core/src/requests-manager.ts (2)
37-37: LGTM: Type safety improvement
Making the promise property required instead of optional is a good change that ensures better type safety and reflects the actual usage in the code.
142-155: LGTM: Improved promise handling
Moving the promise initialization to a microtask and handling failed initial requests immediately are good improvements that:
- Fix the race condition in promise initialization
- Address the core issue where renewed requests failed immediately after a previous failure
- Ensure proper cleanup of resources
cvat-ui/src/actions/export-actions.ts (6)
117-141: LGTM!
The new exportBackupAsync function looks good. It follows a similar structure to exportDatasetAsync and handles the progress updates and error cases appropriately.
170-190: LGTM!
The new listenExportBackupAsync function looks good. It correctly handles the instance type and dispatches the appropriate actions based on the result of the listen function.
81-91: Ensure proper error handling and user feedback for new parameters.
The exportDatasetAsync function now includes new parameters: useDefaultSettings, targetStorage, and an optional name. Make sure to handle any potential errors or edge cases related to these new parameters and provide appropriate user feedback.
Run the following script to verify error handling:
#!/bin/bash
# Description: Verify error handling for new parameters in `exportDatasetAsync`.
# Test: Search for error handling code related to the new parameters.
# Expect: Proper error handling and user feedback for edge cases.
ast-grep --pattern $'exportDatasetAsync($_, $_, $_, useDefaultSettings, targetStorage, name) {
$$$
}'99-107: Verify the behavior of the listen function.
The listen function from core.requests is being used to handle the progress updates for the export request. Please ensure that this function behaves as expected and dispatches the correct actions based on the request status.
Run the following script to verify the behavior of the listen function:
#!/bin/bash
# Description: Verify the behavior of the `listen` function.
# Test: Search for the usage of the `listen` function.
# Expect: Correct actions being dispatched based on the request status.
ast-grep --pattern $'core.requests.listen($_, {
$$$
})'143-168: Verify the behavior of the updated listenExportDatasetAsync function.
The listenExportDatasetAsync function has been updated to align with the changes in exportDatasetAsync. Please ensure that it correctly dispatches the success or failure actions based on the result of the listen function.
Run the following script to verify the behavior of listenExportDatasetAsync:
#!/bin/bash
# Description: Verify the behavior of the updated `listenExportDatasetAsync` function.
# Test: Search for the usage of `listenExportDatasetAsync`.
# Expect: Correct actions being dispatched based on the result of the `listen` function.
ast-grep --pattern $'listenExportDatasetAsync($_, $_, {
$$$
})'8-9: Verify the usage of StorageLocation in the codebase.
The new StorageLocation export from cvat-core-wrapper is being used in this file. Please ensure that it is being used consistently and correctly throughout the codebase.
Run the following script to verify the usage of StorageLocation:
✅ Verification successful
The usage of StorageLocation is consistent and correctly implemented across the codebase
The verification shows that StorageLocation is properly used throughout the codebase:
- It's correctly defined as an enum in both
cvat-core/src/enums.tsandcvat-ui/src/reducers/index.ts - It's consistently used in storage-related components for handling both local and cloud storage locations
- The import in
export-actions.tsaligns with its usage in:- Storage management components
- Import/Export modals
- Task and Project creation forms
- Request handling
The type is properly integrated into the storage management system and follows a consistent pattern across all implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of `StorageLocation` in the codebase.
# Test: Search for the usage of `StorageLocation`.
# Expect: Consistent and correct usage across the codebase.
rg -A 5 $'StorageLocation'
Length of output: 748
Script:
#!/bin/bash
# Let's try a different approach to search for StorageLocation usage
# Search for StorageLocation in import statements
rg "import.*StorageLocation" -A 2
# Search for type annotations or usage of StorageLocation
rg "\bStorageLocation\b" -A 2
# Search for files that might be using StorageLocation
fd -e ts -e tsx -x grep -l "StorageLocation" {}
Length of output: 39201
cvat-ui/src/actions/import-actions.ts (1)
14-15: Imports are correctly added
The new imports from ./requests-actions are appropriate and necessary for the updated functionality.
| ### Fixed | ||
|
|
||
| - Re-newed import/export request failed immediately if the previous failed | ||
| (<https://github.com/cvat-ai/cvat/pull/XXXX>) |
There was a problem hiding this comment.
Update the PR link in the changelog entry.
The changelog entry follows the correct format and provides a clear description of the fix. However, the PR link needs to be updated.
Apply this diff to fix the PR link:
- (<https://github.com/cvat-ai/cvat/pull/XXXX>)
+ (<https://github.com/cvat-ai/cvat/pull/8649>)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Fixed | |
| - Re-newed import/export request failed immediately if the previous failed | |
| (<https://github.com/cvat-ai/cvat/pull/XXXX>) | |
| ### Fixed | |
| - Re-newed import/export request failed immediately if the previous failed | |
| (<https://github.com/cvat-ai/cvat/pull/8649>) |
There was a problem hiding this comment.
@klakhov Yes, please update the link in the changelog entry to point to the correct PR. Replace XXXX with 8649 in the link. Here is the updated entry:
### Fixed
- Re-newed import/export request failed immediately if the previous failed
(<https://github.com/cvat-ai/cvat/pull/8649>)| const rqID = await (instance as Project).annotations.importDataset( | ||
| format, | ||
| useDefaultSettings, | ||
| sourceStorage, | ||
| file, | ||
| { | ||
| convMaskToPoly, | ||
| updateStatusCallback: (message: string, progress: number) => ( | ||
| dispatch(importActions.importDatasetUpdateStatus( | ||
| instance, Math.floor(progress * 100), message, | ||
| )) | ||
| ), | ||
| }); | ||
| if (shouldListenForProgress(rqID, state.requests)) { | ||
| await listen(rqID, dispatch); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| await listenForImport(rqID); | ||
| } else if (instanceType === 'task') { | ||
| dispatch(importActions.importDataset(instance, format)); | ||
| const rqID = await (instance as Task).annotations | ||
| .upload(format, useDefaultSettings, sourceStorage, file, { | ||
| convMaskToPoly, | ||
| }); | ||
| if (shouldListenForProgress(rqID, state.requests)) { | ||
| await listen(rqID, dispatch); | ||
| } | ||
| const rqID = await (instance as Task).annotations.upload( | ||
| format, | ||
| useDefaultSettings, | ||
| sourceStorage, | ||
| file, | ||
| { convMaskToPoly }, | ||
| ); | ||
| await listenForImport(rqID); | ||
| } else { // job | ||
| dispatch(importActions.importDataset(instance, format)); | ||
| const rqID = await (instance as Job).annotations | ||
| .upload(format, useDefaultSettings, sourceStorage, file, { | ||
| convMaskToPoly, | ||
| }); | ||
| const rqID = await (instance as Job).annotations.upload( | ||
| format, | ||
| useDefaultSettings, | ||
| sourceStorage, | ||
| file, | ||
| { convMaskToPoly }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication
The importDatasetAsync function contains similar code blocks for handling project, task, and job instances. Consider refactoring to reduce duplication and improve maintainability.
You could abstract the common logic into a separate function or unify the handling of different instance types.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8649 +/- ##
===========================================
- Coverage 74.37% 74.29% -0.09%
===========================================
Files 401 401
Lines 43422 43420 -2
Branches 3951 3943 -8
===========================================
- Hits 32297 32258 -39
- Misses 11125 11162 +37
|
| ### Fixed | ||
|
|
||
| - Re-newed import/export request failed immediately if the previous failed | ||
| (<https://github.com/cvat-ai/cvat/pull/XXXX>) |
|
Applied comments |
|
Motivation and context
Depends on #8648
To reproduce import annotations to a job for example:
First import must be failed (e.g. wrong archive structure)
Second import must have correct data, but also will be failed
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
StorageLocation, expanding the API surface.Bug Fixes
Documentation
Style