Fixed Issue-#8709 #8840
Fixed Issue-#8709 #8840
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 WalkthroughThe pull request introduces modifications to 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 (
|
![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
🧹 Nitpick comments (2)
cvat-ui/src/components/annotation-page/top-bar/annotation-menu.tsx (2)
74-82: Consider enhancing error handling and user feedbackWhile the implementation is correct, consider these improvements:
- Add error handling for failed state updates
- Make the success message more specific to the state being updated
const changeState = useCallback( (state: JobState) => { - dispatch(updateCurrentJobAsync({ state })).then(() => { - message.info('Job state updated', 2); - setJobState(jobInstance.state); - }); + dispatch(updateCurrentJobAsync({ state })) + .then(() => { + message.success(`Job state updated to ${state}`, 2); + setJobState(jobInstance.state); + }) + .catch((error) => { + message.error(`Failed to update job state: ${error.message}`, 3); + }); }, [jobInstance], );
205-230: Consider adding loading states for better UXWhile the menu structure is good, consider adding loading indicators during state transitions to prevent multiple clicks and provide better user feedback.
const changeJobState = useCallback( (state: JobState) => () => { + const [isLoading, setIsLoading] = useState(false); Modal.confirm({ title: 'Would you like to update current job state?', content: `Job state will be switched to "${state}"`, okText: 'Continue', cancelText: 'Cancel', className: 'cvat-modal-content-change-job-state', - onOk: () => changeState(state), + onOk: async () => { + setIsLoading(true); + try { + await changeState(state); + } finally { + setIsLoading(false); + } + }, + okButtonProps: { + loading: isLoading, + }, }); }, [changeState], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cvat-ui/src/components/annotation-page/top-bar/annotation-menu.tsx(5 hunks)
🔇 Additional comments (3)
cvat-ui/src/components/annotation-page/top-bar/annotation-menu.tsx (3)
27-28: LGTM: Import statements are correctly added
The new imports align with the PR's objective to handle job state updates.
Line range hint 56-62: Perfect fix for the UI inconsistency issue
This change directly addresses the reported issue (#8709) by ensuring the UI state is updated immediately when a job is completed. The addition of a success message provides good user feedback.
134-170: LGTM: Modal UI structure improved
The restructuring of the Collapse component using the items prop improves code readability while maintaining the same functionality.
|
Motivation and context
The change is required to provide a consistent UI. There's some flaws in UI. After the current job is switched to the completed state automatically, it still shows the previous state in UI.
How has this been tested?
Only a single react state has been set.
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
Bug Fixes
Refactor