Improved save annotations action #8501
Conversation
WalkthroughThe changes involve a modification to the Changes
Sequence Diagram(s)sequenceDiagram
participant A as User
participant B as AnnotationService
participant C as JobUpdater
A->>B: Save Annotations
B->>C: Dispatch updateCurrentJobAsync
C-->>B: Job state updated
B-->>A: Annotations saved successfully
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 (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cvat-ui/src/actions/annotation-actions.ts (2)
1014-1017: Consider adding explicit error handling for job status update.While the overall error handling in this function is good, it might be beneficial to add explicit error handling for the job status update operation. This could help in identifying and addressing any issues specific to the status update process.
For example:
try { await dispatch(updateCurrentJobAsync({ state: JobState.IN_PROGRESS })); } catch (error) { // Log the error or handle it appropriately console.error('Failed to update job status:', error); // Optionally, you might want to throw the error or handle it in a way that doesn't break the overall save process }This addition would provide more granular error handling and potentially improve debugging and error recovery.
Line range hint
1-1024: Reminder: Address unchecked items in the PR checklistThe changes made in this file look good and address the specific issue mentioned in the PR objectives. However, I noticed that several items in the PR checklist remain unchecked, including:
- Creation of a changelog fragment
- Updates to documentation
- Addition of tests to cover the changes made
These are important steps in the development process. Could you please address these items? Specifically:
- Create a changelog entry to document this fix.
- Update any relevant documentation that might be affected by this change.
- Add or update tests to ensure the new behavior is correctly covered.
Completing these steps will help maintain the project's quality and make it easier for other developers to understand and verify the changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- cvat-ui/src/actions/annotation-actions.ts (1 hunks)
🔇 Additional comments (1)
cvat-ui/src/actions/annotation-actions.ts (1)
1015-1015: Excellent fix for the job status update issue.The addition of the
awaitkeyword here is crucial. It ensures that the job status update is completed before the function continues execution. This change directly addresses the issue mentioned in the PR objectives and should resolve the problems with thebeforeJobFinishcallbacks that require a valid job updated date.This improvement enhances the reliability of the annotation saving process by guaranteeing that all necessary updates are completed before any dependent actions are executed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8501 +/- ##
===========================================
- Coverage 74.36% 74.34% -0.02%
===========================================
Files 394 394
Lines 42177 42177
Branches 3896 3896
===========================================
- Hits 31364 31358 -6
- Misses 10813 10819 +6
|
Motivation and context
Inside of save annotations action we do not wait for update job status request to complete. That leads to problems with
beforeJobFinishcallbacks. Some of them require valid job updated date.How has this been tested?
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit