Fixed disabling the requests, improved notification links
#8197
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 series of changes enhances the CVAT application's handling of requests by introducing a new action for disabling requests and updating various components to use a custom markdown renderer. The updates improve error message formatting and state management, incorporating new props for better functionality. Overall, these modifications aim to streamline request management and enhance user notifications within the application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant RequestsActions
participant RequestsReducer
User->>App: Trigger request disable
App->>RequestsActions: Dispatch DISABLE_REQUEST
RequestsActions-->>RequestsReducer: Update state to disable request
RequestsReducer-->>App: New state with disabled request
App-->>User: Show updated request status
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 as PR comments)
Additionally, you can add 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, codebase verification and nitpick comments (1)
cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/tools-control.tsx (1)
790-790: Integration ofCVATMarkdownis missing inopencv-control.tsx.The
checkTrackedStatesfunction intools-control.tsxis correctly updated to useCVATMarkdown. However, theopencv-control.tsxfile does not integrateCVATMarkdownat all. Please ensure thatCVATMarkdownis used consistently for error handling in both files.
opencv-control.tsxneeds to be updated to useCVATMarkdownsimilarly totools-control.tsx.Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all instances of
checkTrackedStatesare updated to handle the newCVATMarkdowncomponent correctly.
Let's correct the file type and re-run the search to verify the usage of
checkTrackedStates.
To ensure that
CVATMarkdownis correctly integrated withcheckTrackedStates, let's search forCVATMarkdownusage within theopencv-control.tsxandtools-control.tsxfiles.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instances of `checkTrackedStates` in the codebase. # Test: Search for the function usage. Expect: Instances of `checkTrackedStates` to be updated. rg --type python -A 5 $'checkTrackedStates'Length of output: 73
Script:
#!/bin/bash # Description: Verify all instances of `checkTrackedStates` in the codebase. # Test: Search for the function usage. Expect: Instances of `checkTrackedStates` to be updated. rg --type ts -A 5 'checkTrackedStates'Length of output: 3049
Script:
#!/bin/bash # Description: Verify the usage of `CVATMarkdown` in the relevant files. # Test: Search for `CVATMarkdown` usage in the specified files. rg 'CVATMarkdown' cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/opencv-control.tsx cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/tools-control.tsxLength of output: 1414
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- cvat-ui/src/actions/requests-actions.ts (2 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/tools-control.tsx (7 hunks)
- cvat-ui/src/components/common/cvat-markdown.tsx (1 hunks)
- cvat-ui/src/components/cvat-app.tsx (6 hunks)
- cvat-ui/src/components/export-backup/export-backup-modal.tsx (4 hunks)
- cvat-ui/src/components/export-dataset/export-dataset-modal.tsx (4 hunks)
- cvat-ui/src/components/import-dataset/import-dataset-modal.tsx (4 hunks)
- cvat-ui/src/components/requests-page/request-card.tsx (6 hunks)
- cvat-ui/src/components/requests-page/requests-list.tsx (1 hunks)
- cvat-ui/src/reducers/index.ts (1 hunks)
- cvat-ui/src/reducers/notifications-reducer.ts (2 hunks)
- cvat-ui/src/reducers/requests-reducer.ts (2 hunks)
- cvat-ui/src/styles.scss (1 hunks)
Files skipped from review due to trivial changes (2)
- cvat-ui/src/reducers/notifications-reducer.ts
- cvat-ui/src/styles.scss
Additional comments not posted (28)
cvat-ui/src/components/common/cvat-markdown.tsx (4)
1-4: LGTM! Import statements are appropriate.The import statements include necessary libraries and components.
6-6: LGTM! Type definition is clear and correct.The type alias
UseHistoryTypeis defined correctly.
8-27: LGTM! Higher-order component implementation is clear and correct.The HOC
RouterLinkHOCcorrectly handles internal and external links.
29-41: LGTM! Function implementation is clear and correct.The function
CVATMarkdowncorrectly integrates custom link handling withReactMarkdown.cvat-ui/src/reducers/requests-reducer.ts (2)
14-14: LGTM! State changes are clear and correct.The addition of the
disabledarray indefaultStatealigns with the new functionality.
36-42: LGTM! Reducer changes are clear and correct.The new case for
DISABLE_REQUESTcorrectly updates thedisabledarray in the state.cvat-ui/src/components/requests-page/requests-list.tsx (2)
36-36: LGTM! State usage changes are clear and correct.The
disabledstate property is correctly retrieved from the Redux store.
39-44: LGTM! Request rendering changes are clear and correct.The
RequestCardcomponent correctly receives adisabledprop based on thedisabledstate property.cvat-ui/src/actions/requests-actions.ts (2)
24-24: LGTM! New action typeDISABLE_REQUESTadded toRequestsActionsTypes.The addition follows the existing pattern and enhances the set of actions for request management.
48-50: LGTM! New action creatordisableRequestadded torequestsActions.The addition follows the existing pattern and correctly uses the
createActionfunction.cvat-ui/src/components/export-backup/export-backup-modal.tsx (2)
Line range hint
8-18:
LGTM! Import statements updated to useCVATMarkdownanduseHistoryhook.The changes follow the existing pattern and enhance the component's functionality.
105-109: LGTM! Notification logic updated to useCVATMarkdownwithhistoryprop.The changes follow the existing pattern and enhance the component's functionality.
cvat-ui/src/components/export-dataset/export-dataset-modal.tsx (2)
Line range hint
9-20:
LGTM! Import statements updated to useCVATMarkdownanduseHistoryhook.The changes follow the existing pattern and enhance the component's functionality.
126-126: LGTM! Notification logic updated to useCVATMarkdownwithhistoryprop.The changes follow the existing pattern and enhance the component's functionality.
cvat-ui/src/components/requests-page/request-card.tsx (6)
23-23: LGTM!The import statement for
requestsActionsis appropriate and necessary for the new functionality.
28-28: LGTM!The addition of the
disabledprop to thePropsinterface is appropriate and aligns with the new functionality.
141-141: LGTM!Destructuring the
disabledprop frompropsis necessary and correctly implemented.
156-158: LGTM!Adjusting the
styleobject based on thedisabledstate is a good approach to visually indicate the disabled state.
170-170: LGTM!Dispatching the
requestsActions.disableRequest(request)action after downloading is appropriate and necessary.
182-182: LGTM!Dispatching the
requestsActions.disableRequest(request)action after canceling is appropriate and necessary.cvat-ui/src/reducers/index.ts (1)
956-956: LGTM!Renaming the
urlsproperty todisabledin theRequestsStateinterface is appropriate and aligns with the new functionality.cvat-ui/src/components/import-dataset/import-dataset-modal.tsx (3)
9-9: LGTM!The import statements for
useHistoryandCVATMarkdownare appropriate and necessary for the new functionality.Also applies to: 22-22
282-282: LGTM!Using the
useHistoryhook to get thehistoryobject is appropriate and necessary for navigation purposes.
471-471: LGTM!Using the
CVATMarkdowncomponent to render the notification description is appropriate and likely provides additional functionality or styling specific to the CVAT application.cvat-ui/src/components/cvat-app.tsx (2)
361-369: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all instances of
showMessagesare updated to handle the newCVATMarkdowncomponent correctly.Verification successful
Let's correct the file type and function name, and re-run the script to verify the usage of
showMessagein the codebase.
Let's update the script to search within both
tsandtsxfiles to verify the usage ofshowMessagein the codebase.
The
showMessagefunction is correctly used in the codebase.The function
showMessageis defined and utilized appropriately withincvat-ui/src/components/cvat-app.tsx. Other instances found in the codebase are related to a different function (showMessageWithPause) and do not impact theshowMessagefunction.
cvat-ui/src/components/cvat-app.tsxcvat-core/src/annotations-actions.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instances of `showMessages` in the codebase. # Test: Search for the function usage. Expect: Instances of `showMessages` to be updated. rg --type python -A 5 $'showMessages'Length of output: 67
Script:
#!/bin/bash # Description: Verify all instances of `showMessage` in the codebase. # Test: Search for the function usage. Expect: Instances of `showMessage` to be updated. rg --type tsx -A 5 'showMessage'Length of output: 60
Script:
#!/bin/bash # Description: Verify all instances of `showMessage` in the codebase. # Test: Search for the function usage. Expect: Instances of `showMessage` to be updated. rg --type-add 'ts:*.{ts,tsx}' -A 5 'showMessage'Length of output: 2836
Line range hint
392-408:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all instances of
showErrorsare updated to handle the newCVATMarkdowncomponent correctly.Verification successful
LGTM! All instances of
showErrorshave been updated to handle the newCVATMarkdowncomponent correctly.
cvat-ui/src/components/cvat-app.tsx: TheshowErrorsfunction is defined and used within this file, and it correctly utilizes theCVATMarkdowncomponent.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instances of `showErrors` in the codebase. # Test: Search for the function usage. Expect: Instances of `showErrors` to be updated. rg --type python -A 5 $'showErrors'Length of output: 65
Script:
#!/bin/bash # Description: Verify all instances of `showErrors` in the codebase. # Test: Search for the function usage. Expect: Instances of `showErrors` to be updated. rg --type ts,tsx -A 5 $'showErrors'Length of output: 65
Script:
#!/bin/bash # Description: Verify all instances of `showErrors` in the codebase. # Test: Search for the function usage. Expect: Instances of `showErrors` to be updated. rg --type typescript,tsx -A 5 $'showErrors'Length of output: 81
Script:
#!/bin/bash # Description: Verify all instances of `showErrors` in the codebase. # Test: Search for the function usage. Expect: Instances of `showErrors` to be updated. rg --type ts,tsx -A 5 'showErrors'Length of output: 65
Script:
#!/bin/bash # Description: Verify all instances of `showErrors` in the codebase. # Test: Search for the function usage. Expect: Instances of `showErrors` to be updated. rg --type-add 'typescript:*.ts' --type-add 'tsx:*.tsx' -t typescript -t tsx -A 5 'showErrors'Length of output: 1049
cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/tools-control.tsx (2)
443-443: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all instances of
runInteractionRequestare updated to handle the newCVATMarkdowncomponent correctly.Verification successful
Verification successful.
All instances of
runInteractionRequestare correctly updated to handle the newCVATMarkdowncomponent within the same file.
cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/tools-control.tsxScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instances of `runInteractionRequest` in the codebase. # Test: Search for the function usage. Expect: Instances of `runInteractionRequest` to be updated. rg --type python -A 5 $'runInteractionRequest'Length of output: 76
Script:
#!/bin/bash # Description: Verify all instances of `runInteractionRequest` in the codebase. # Test: Search for the function usage. Expect: Instances of `runInteractionRequest` to be updated. rg --type tsx -A 5 $'runInteractionRequest'Length of output: 70
Script:
#!/bin/bash # Description: Verify all instances of `runInteractionRequest` in the codebase. # Test: Search for the function usage. Expect: Instances of `runInteractionRequest` to be updated. rg 'runInteractionRequest' --glob '*.tsx' -A 5Length of output: 2612
536-536: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all instances of
onTrackingare updated to handle the newCVATMarkdowncomponent correctly.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8197 +/- ##
========================================
Coverage 83.37% 83.38%
========================================
Files 388 388
Lines 41315 41319 +4
Branches 3855 3856 +1
========================================
+ Hits 34447 34452 +5
+ Misses 6868 6867 -1
|
|
| @@ -0,0 +1,41 @@ | |||
| import Button from 'antd/lib/button'; | |||
|
|
||
| const RouterLinkHOC = (history?: UseHistoryType) => ( | ||
| function (props: { children: React.ReactNode, href?: string }): JSX.Element { | ||
| const { href, children } = props; |
There was a problem hiding this comment.
Why not put useHistory here?
In this case, in external components we do not need to have history.
There was a problem hiding this comment.
Yes, that would be better. But we cant use useHistoty(its undefined). The notifications rendered by antd notification module are outside of our Router. It seems thats because its created using React.render docs
| @@ -33,10 +33,17 @@ function RequestsList(props: Props): JSX.Element { | |||
| const dispatch = useDispatch(); | |||
| const { query, count } = props; | |||
| const { page } = query; | |||
| const { requests } = useSelector((state: CombinedState) => state.requests); | |||
| const { requests, disabled } = useSelector((state: CombinedState) => state.requests); | |||
There was a problem hiding this comment.
useSelector((state: CombinedState) => { requests: state.requests, disabled: state.disabled }, shallowEqual);
would be more optimized as state.requests may change more often than state.requests.requests and state.requests.disabled.
Motivation and context
The pr adresses two problems:
<a>tags, using which reloads the page.How has this been tested?
Manual
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)(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
CVATMarkdowncomponent for improved rendering of markdown content in error notifications.Enhancements
ReactMarkdownwithCVATMarkdownacross various components for consistent error message formatting.disabledprop toRequestCardcomponent to manage interactive states.Bug Fixes
Styles