Fix: Keybinds in UI allow drawing disabled shape types #8685
Conversation
WalkthroughThis pull request implements changes to address an issue in the user interface where users could draw disabled shape types using keybinds. The modifications include updates to the Changes
Assessment against linked issues
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: 0
🧹 Outside diff range and nitpick comments (3)
changelog.d/20241112_132508_klakhov_fix_remember_object.md (1)
1-4: Consider enhancing the changelog entry with more specific details.While the current entry correctly describes the fix, consider adding more context to help users better understand the scope of the change:
### Fixed -Keybinds in UI allow drawing disabled shape types +Keybinds in UI no longer allow drawing disabled shape types in Standard mode +when certain shape types are not available for the selected label (<https://github.com/cvat-ai/cvat/pull/8685>)cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx (1)
Line range hint
12-24: Consider adding error boundary protectionWhile the component's state management is robust, consider wrapping it with an error boundary to gracefully handle potential runtime errors, especially during canvas operations.
Example implementation:
+import ErrorBoundary from 'components/error-boundary/error-boundary'; function SingleShapeSidebar(): JSX.Element { + return ( + <ErrorBoundary> {/* existing component content */} + </ErrorBoundary> + ); }cvat-ui/src/reducers/annotation-reducer.ts (1)
34-42: LGTM! Consider documenting the default behavior.The function implementation is clean and handles all cases correctly. The null return for TAG type will help prevent drawing disabled shape types.
Consider adding a JSDoc comment to document the default behavior:
+/** + * Determines the shape type for a given label + * @param label - The label to check + * @returns The shape type if available, null for TAG type, or RECTANGLE as default + */ export function labelShapeType(label?: Label): ShapeType | null {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
changelog.d/20241112_132508_klakhov_fix_remember_object.md(1 hunks)cvat-ui/src/actions/annotation-actions.ts(1 hunks)cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx(2 hunks)cvat-ui/src/reducers/annotation-reducer.ts(3 hunks)cvat-ui/src/reducers/index.ts(1 hunks)
🔇 Additional comments (7)
cvat-ui/src/reducers/index.ts (1)
772-772: LGTM! The nullable shape type enables proper handling of disabled states.
Making activeShapeType nullable is the right approach as it allows the UI to explicitly represent when no shape type should be active, which is necessary for handling disabled shape types.
Let's verify that components properly handle the null case:
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information about how activeShapeType is handled across the codebase.
The null case for activeShapeType is properly handled throughout the codebase
The codebase already has robust handling of the null case for activeShapeType:
- In
annotation-reducer.ts, it correctly initializesactiveShapeTypeas null when dealing with tag labels - The drawing control mapping in
annotation-reducer.tsonly processes defined shape types, safely handling the null case - The
rememberObjectaction inlabels-list.tsxhas proper type narrowing logic that considers shape type constraints - The setup tag popover explicitly sets
activeShapeTypeto undefined, showing awareness of non-shape states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that use activeShapeType to ensure they handle null
rg -l "activeShapeType" --type ts --type tsx | xargs rg -A 5 "activeShapeType"
Length of output: 11958
cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx (3)
24-24: LGTM: Import addition aligns with PR objective
The addition of labelShapeType import is appropriate for determining valid shape types from labels, which helps prevent drawing of disabled shapes.
Line range hint 431-435: LGTM: Shortcut handlers are safe with shape type validation
The SWITCH_DRAW_MODE_SINGLE_SHAPE handler safely disables drawing mode. Since shape type validation occurs during canvas initialization, it's not possible to bypass the validation through shortcuts.
263-263: LGTM: Core fix for keybind shape type validation
The use of labelShapeType ensures that only valid shape types for the selected label can be activated, effectively preventing the drawing of disabled shapes through keybinds.
Let's verify the fix by checking the implementation of labelShapeType:
✅ Verification successful
LGTM: Verified shape type validation logic
The labelShapeType function in cvat-ui/src/reducers/annotation-reducer.ts correctly validates shape types by:
- Checking if the label's type matches any valid
ShapeTypevalue - Returning
nullfor tag-type labels - Ensuring only valid shape types are returned
This implementation confirms that the keybind fix properly prevents drawing disabled shapes by validating against the label's constraints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for labelShapeType implementation to verify its logic
ast-grep --pattern 'export const labelShapeType = ($_) => {
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Let's try different patterns to find labelShapeType implementation
# Try without arrow function syntax
ast-grep --pattern 'export function labelShapeType($_) {
$$$
}'
# Search for any labelShapeType definition
ast-grep --pattern 'labelShapeType = $_'
# Use ripgrep as fallback to find any occurrence
rg "labelShapeType" -A 5
Length of output: 3176
cvat-ui/src/reducers/annotation-reducer.ts (2)
198-202: LGTM! Changes properly integrate the new shape type handling.
The initialization of activeShapeType to null and subsequent setting via labelShapeType correctly implements the fix for disabled shape types.
198-202: Verify handling of null activeShapeType in UI components.
Let's ensure that UI components and keyboard shortcuts properly handle the case when activeShapeType is null.
✅ Verification successful
Null activeShapeType is properly handled in the codebase
The code analysis shows that activeShapeType being null is a valid state and is properly handled:
- When
activeShapeTypeis null, it's explicitly set for tags insetup-tag-popover.tsxwithactiveShapeType: undefined - The shape type control mapping in
annotation-reducer.tsonly activates controls whenactiveShapeTypeis defined - Drawing operations in
annotation-actions.tscheck foractiveObjectType !== ObjectType.TAGbefore usingactiveShapeType - UI components properly handle the null case by checking object types first
The initial concern about null handling is unfounded as the codebase has proper type checks and conditional logic in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UI components that use activeShapeType
echo "Searching for activeShapeType usage in components:"
rg "activeShapeType" --type ts --type tsx
# Search for keyboard shortcut handlers
echo "Searching for keyboard shortcut handlers:"
rg -B 2 -A 2 "keydown|keyup|keypress" --type ts --type tsx
Length of output: 362
Script:
#!/bin/bash
# Search for activeShapeType usage without file type restrictions
echo "Searching for activeShapeType usage:"
rg "activeShapeType" -l
# Search for keyboard shortcut handlers without file type restrictions
echo "Searching for keyboard shortcut handlers:"
rg "keydown|keyup|keypress" -l
# Get content of files containing activeShapeType for detailed analysis
echo "Content of files with activeShapeType:"
rg "activeShapeType" -C 5
Length of output: 22558
cvat-ui/src/actions/annotation-actions.ts (1)
1084-1084: LGTM! Type change addresses the keybind issue.
The change to allow null for activeShapeType is a good solution that prevents drawing of disabled shape types through keybinds, directly addressing the issue mentioned in #8673.
Let's verify the usage of this parameter across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response:
All call sites handle nullable activeShapeType correctly
The verification shows that all usages of rememberObject properly handle the activeShapeType parameter:
- Tag-related calls correctly pass
undefinedor omit the parameter - Shape-related calls pass specific shape types
- Label-list component handles type transitions safely with conditional logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of rememberObject's activeShapeType parameter
# to ensure all call sites handle the nullable type correctly
# Search for rememberObject calls
rg -A 5 'rememberObject\('
# Search for related type definitions
ast-grep --pattern 'type $_ = { $$$; activeShapeType?: $_; $$$; }'
Length of output: 7644
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8685 +/- ##
===========================================
+ Coverage 74.20% 74.21% +0.01%
===========================================
Files 401 401
Lines 43504 43510 +6
Branches 3950 3950
===========================================
+ Hits 32280 32293 +13
+ Misses 11224 11217 -7
|
Motivation and context
Resolved: #8673
How has this been tested?
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation