Enhanced running of annotations actions #8727
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 update introduces significant enhancements to the annotation actions functionality within the CVAT application. Key features include the ability to undo and redo changes in annotation actions, support for various object types in annotation actions, and the capability to execute actions on specific objects. Additionally, several classes and methods related to annotation actions have been restructured or added, improving the overall flexibility and usability of the annotation tools. The changes also involve modifications to the API and UI components to accommodate these new features. 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: 16
🧹 Outside diff range and nitpick comments (32)
changelog.d/20241120_234852_sekachev.bs_updated_annotations_actions.md (1)
3-4: Expand the changelog entry with more detailsWhile the entry follows the correct format, consider adding more details about:
- Types of objects that can be targeted
- Examples of supported actions
- Any limitations or prerequisites
### Added -Ability to run annotations action on a certain object +Ability to run annotations action on a specific object, including: +- Support for [list supported object types] +- Capability to execute [list supported actions] +- [Any important limitations or prerequisites] (<https://github.com/cvat-ai/cvat/pull/8727>)changelog.d/20241120_234543_sekachev.bs_updated_annotations_actions.md (1)
3-3: Fix grammatical issue in the changelog entry.The phrase "an annotations action" is grammatically incorrect. It should be either singular or plural, but not mixed.
-A user may undo or redo changes, made by an annotations action +A user may undo or redo changes made by an annotation action🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: The plural noun “annotations” cannot be used with the article “an”. Did you mean “an annotation” or “annotations”?
Context: ... user may undo or redo changes, made by an annotations action (<https://github.com/cvat-ai/c...(A_NNS)
cvat-core/src/annotations-actions/remove-filtered-shapes.ts (2)
8-16: Consider adding JSDoc comments for better documentation.While the empty
init()anddestroy()methods are fine, adding JSDoc comments would better document their purpose and potential future use, especially since this is part of a public API.Consider adding documentation like this:
+/** + * Initializes the RemoveFilteredShapes action. + * Currently a no-op, but may be extended in the future. + */ public async init(): Promise<void> { // nothing to init } +/** + * Cleans up resources used by the RemoveFilteredShapes action. + * Currently a no-op, but may be extended in the future. + */ public async destroy(): Promise<void> { // nothing to destroy }
29-41: LGTM! Clear implementation of helper methods.The helper methods are well-implemented with clear intent. The comment in
isApplicableForObjecteffectively explains why the method returnsfalse.However, if the class remains as-is (removing all shapes without filtering), consider updating the
namegetter to return a more accurate description like 'Remove all shapes'.public get name(): string { - return 'Remove filtered shapes'; + return 'Remove all shapes'; }cvat-core/src/annotations-actions/annotations-actions.ts (3)
26-26: Fix the linting errorRemove the extra space before the parenthesis to comply with the style guide.
- if (!(action instanceof BaseAction) ) { + if (!(action instanceof BaseAction)) {🧰 Tools
🪛 eslint
[error] 26-26: There should be no space before this paren.
(space-in-parens)
🪛 GitHub Check: Linter
[failure] 26-26:
There should be no space before this paren
25-36: Consider making action registration thread-safeThe current implementation might face race conditions if multiple actions are registered concurrently. Consider making the registration process atomic.
Consider using a synchronization mechanism or making the registration process synchronous by removing the async keyword since there are no async operations:
-export async function registerAction(action: BaseAction): Promise<void> { +export function registerAction(action: BaseAction): void {🧰 Tools
🪛 eslint
[error] 26-26: There should be no space before this paren.
(space-in-parens)
🪛 GitHub Check: Linter
[failure] 26-26:
There should be no space before this paren
114-114: Remove extra blank lines at end of fileRemove the extra blank lines at the end of the file to comply with the linting rules.
} - -🧰 Tools
🪛 eslint
[error] 114-115: Too many blank lines at the end of file. Max of 0 allowed.
(no-multiple-empty-lines)
🪛 GitHub Check: Linter
[failure] 114-114:
Too many blank lines at the end of file. Max of 0 allowedcvat-ui/src/cvat-core-wrapper.ts (1)
44-46: LGTM! Good architectural improvement with specialized action classes.The introduction of
BaseShapesAction,BaseCollectionAction, andBaseActionrepresents a well-structured approach to handling different types of annotation actions. This modular design improves code organization and maintainability.This architectural change allows for better separation of concerns:
BaseAction: Generic action handlingBaseShapesAction: Shape-specific operationsBaseCollectionAction: Collection-wide operationscvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item-basics.tsx (2)
50-50: Consider adding parameters torunAnnotationActionThe method signature
runAnnotationAction(): voidis very generic. Consider adding parameters to specify which annotation action to run, improving type safety and making the API more explicit.Example suggestion:
- runAnnotationAction(): void; + runAnnotationAction(actionType: AnnotationActionType, options?: AnnotationActionOptions): void;
171-171: Consider maintaining alphabetical ordering of propsTo maintain consistency with the rest of the codebase and improve maintainability, consider keeping the props in alphabetical order.
resetCuboidPerspective, + runAnnotationAction, setColorPickerVisible, edit, slice, - runAnnotationAction,cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item.tsx (1)
44-44: Add JSDoc documentation for the new method.Please add documentation explaining the purpose and behavior of
runAnnotationAction. This will help other developers understand when and how to use this method.+ /** Executes an annotation action for the current object */ runAnnotationAction(): void;cvat-core/src/index.ts (2)
37-44: Fix missing semicolons and verify action importsThe new imports align well with the enhanced annotation actions functionality. However, there are style issues to address.
Add missing semicolons after the import statements:
-import { BaseCollectionAction } from './annotations-actions/base-collection-action' -import { BaseShapesAction } from './annotations-actions/base-shapes-action' +import { BaseCollectionAction } from './annotations-actions/base-collection-action'; +import { BaseShapesAction } from './annotations-actions/base-shapes-action';🧰 Tools
🪛 eslint
[error] 43-44: Missing semicolon.
(@typescript-eslint/semi)
37-44: Well-structured enhancement to annotation actionsThe changes demonstrate good architectural decisions:
- Clear separation of concerns in the action handling system
- Modular approach with base classes for different action types
- Consistent interface updates
Consider adding JSDoc comments to document the new action system's usage patterns and examples for future contributors.
Also applies to: 175-176, 220-221
🧰 Tools
🪛 eslint
[error] 43-44: Missing semicolon.
(@typescript-eslint/semi)
cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item-menu.tsx (1)
50-50: Add TypeScript type information for runAnnotationAction.The
runAnnotationActionprop should have its return type specified for better type safety and documentation.- runAnnotationAction(): void; + runAnnotationAction: () => void;cvat-core/src/object-state.ts (1)
Line range hint
528-547: Consider enhancing error handling in export implementationWhile the implementation follows the established pattern, consider adding type validation or error handling for cases where the internal export handler is missing but required.
Consider this enhancement:
value: function exportImplementation(): ObjectState { if (this.__internal && this.__internal.export) { return this.__internal.export(this); } + // Consider adding a warning or throwing an error if export is required but not available + // console.warn('Export handler not available for this object state'); return this; },cvat-core/src/session.ts (1)
175-178: Fix line length to improve readabilityThe line exceeds the maximum length of 120 characters. Consider breaking it into multiple lines.
- async commit(added, removed, frame) { - const result = await PluginRegistry.apiWrapper.call(this, prototype.annotations.commit, added, removed, frame); - return result; - }, + async commit(added, removed, frame) { + const result = await PluginRegistry.apiWrapper.call( + this, + prototype.annotations.commit, + added, + removed, + frame, + ); + return result; + },🧰 Tools
🪛 eslint
[error] 176-176: This line has a length of 131. Maximum allowed is 120.
(max-len)
cvat-core/src/annotations-actions/propagate-shapes.ts (1)
38-39: Refactor long lines to comply with the maximum line lengthLines 38 and 39 exceed the maximum allowed line length of 120 characters, as indicated by the static analysis tools. Refactoring these lines will improve readability and adhere to the project's coding standards.
Apply this diff to split the lines appropriately:
- const frameNumbers = this.#instance instanceof Job ? await this.#instance.frames.frameNumbers() : range(0, this.#instance.size); + const frameNumbers = this.#instance instanceof Job + ? await this.#instance.frames.frameNumbers() + : range(0, this.#instance.size); - const propagatedShapes = propagateShapes<SerializedShape>(collection.shapes, number, this.#targetFrame, frameNumbers); + const propagatedShapes = propagateShapes<SerializedShape>( + collection.shapes, + number, + this.#targetFrame, + frameNumbers, + );🧰 Tools
🪛 eslint
[error] 38-38: This line has a length of 136. Maximum allowed is 120.
(max-len)
[error] 39-39: This line has a length of 126. Maximum allowed is 120.
(max-len)
cvat-core/src/annotations-actions/base-collection-action.ts (3)
5-5: Remove unused import 'omit'The
omitfunction is imported from 'lodash' but is not used in this file. Unused imports can clutter the code and may cause confusion.Apply this diff to remove the unused import:
-import { throttle, omit } from 'lodash'; +import { throttle } from 'lodash';🧰 Tools
🪛 GitHub Check: Linter
[failure] 5-5:
'omit' is defined but never used🪛 eslint
[error] 5-5: 'omit' is defined but never used.
(@typescript-eslint/no-unused-vars)
139-152: Correct indentation to improve code readabilityThere are inconsistencies in indentation from lines 139 to 152, which can reduce code readability and maintainability.
Apply this diff to correct the indentation:
.reduce<CollectionActionInput['collection']>((acc, value, idx) => { - if (states[idx].objectType === ObjectType.SHAPE) { - acc.shapes.push(value as SerializedShape); - } - if (states[idx].objectType === ObjectType.TAG) { - acc.tags.push(value as SerializedTag); - } - if (states[idx].objectType === ObjectType.TRACK) { - acc.tracks.push(value as SerializedTrack); - } - return acc; - }, { shapes: [], tags: [], tracks: [] }); + .reduce<CollectionActionInput['collection']>((acc, value, idx) => { + if (states[idx].objectType === ObjectType.SHAPE) { + acc.shapes.push(value as SerializedShape); + } + + if (states[idx].objectType === ObjectType.TAG) { + acc.tags.push(value as SerializedTag); + } + + if (states[idx].objectType === ObjectType.TRACK) { + acc.tracks.push(value as SerializedTrack); + } + + return acc; + }, { shapes: [], tags: [], tracks: [] });🧰 Tools
🪛 GitHub Check: Linter
[failure] 139-139:
Expected indentation of 12 spaces but found 16
[failure] 140-140:
Expected indentation of 16 spaces but found 20
[failure] 141-141:
Expected indentation of 12 spaces but found 16
[failure] 143-143:
Expected indentation of 12 spaces but found 16
[failure] 144-144:
Expected indentation of 16 spaces but found 20🪛 eslint
[error] 139-139: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 140-140: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 141-141: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 143-143: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 144-144: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 145-145: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 147-147: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 148-148: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 149-149: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 151-151: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 152-152: Expected indentation of 8 spaces but found 12.
(@typescript-eslint/indent)
155-155: Remove extra space before 'frameData'Multiple spaces are found before
frameDatain the object passed toaction.applyFilter. This may affect code readability.Apply this diff to correct the spacing:
- const filteredByAction = action.applyFilter({ collection: exportedCollection, frameData }); + const filteredByAction = action.applyFilter({ collection: exportedCollection, frameData });🧰 Tools
🪛 eslint
[error] 155-155: Multiple spaces found before 'frameData'.
(no-multi-spaces)
cvat-core/src/annotations-actions/base-shapes-action.ts (4)
5-5: Remove unused import 'omit' to clean up code.The import
'omit'from'lodash'is defined but never used in the file. Removing it will eliminate unnecessary code and prevent potential confusion.Apply this diff to remove the unused import:
-import { omit, throttle } from 'lodash'; +import { throttle } from 'lodash';🧰 Tools
🪛 eslint
[error] 5-5: 'omit' is defined but never used.
(@typescript-eslint/no-unused-vars)
79-79: Line exceeds the maximum allowed length of 120 characters.Line 79 has a length of 123 characters, which exceeds the maximum allowed length. Splitting the line enhances readability and complies with style guidelines.
Apply this diff to split the line:
-const filteredShapesByFrame = exportedCollection.shapes.reduce<Record<number, SerializedShape[]>>((acc, shape) => { +const filteredShapesByFrame = exportedCollection.shapes + .reduce<Record<number, SerializedShape[]>>((acc, shape) => {🧰 Tools
🪛 eslint
[error] 79-79: This line has a length of 123. Maximum allowed is 120.
(max-len)
91-92: Refactor duplicate code for obtainingframeDatainto a helper function.The logic for retrieving
frameDatais duplicated in both therunandcallfunctions. Extracting this code into a shared helper function adheres to the DRY (Don't Repeat Yourself) principle and improves maintainability.Duplicated code:
// In run function const frameData = await Object.getPrototypeOf(instance).frames .get.implementation.call(instance, frame); // In call function const frameData = await Object.getPrototypeOf(instance).frames.get.implementation.call(instance, frame);Also applies to: 170-170
171-171: Adjust spacing to correct multiple spaces before 'frameData'.There are multiple spaces before
'frameData'. Correcting the spacing enhances code consistency and adheres to the coding standards.Apply this diff to fix the spacing:
-const filteredByAction = action.applyFilter({ collection: { shapes: exported }, frameData }); +const filteredByAction = action.applyFilter({ collection: { shapes: exported }, frameData });🧰 Tools
🪛 eslint
[error] 171-171: Multiple spaces found before 'frameData'.
(no-multi-spaces)
cvat-core/src/annotations-filter.ts (1)
62-64: Simplify conditional statements by removing unnecessary 'else' blocksAfter a
returnstatement in anifblock, theelseis unnecessary. Removing it improves code readability.Apply this diff to refactor the code:
if (spec.inputType === AttributeType.NUMBER) { return [name, +value]; } - else if (spec.inputType === AttributeType.CHECKBOX) { + if (spec.inputType === AttributeType.CHECKBOX) { return [name, value === 'true']; } return [name, value];🧰 Tools
🪛 eslint
[error] 62-64: Unnecessary 'else' after 'return'.
(no-else-return)
cvat-ui/src/components/annotation-page/annotations-actions/annotations-actions-modal.tsx (4)
227-227: Destructure 'props' in the function signatureFor improved readability and to adhere to React best practices, destructure
propsdirectly in the function signature ofActionParameterComponent.Apply this diff:
- function ActionParameterComponent(props: ActionParameterProps & { onChange: (value: string) => void }): JSX.Element { + function ActionParameterComponent({ defaultValue, type, values, onChange }: ActionParameterProps & { onChange: (value: string) => void }): JSX.Element {Remove the destructuring of
propswithin the function body accordingly.
279-279: Destructure 'props' in the function signatureTo comply with React guidelines and resolve the linting error, destructure
propsin the function signature ofAnnotationsActionsModalContent.Apply this diff:
- function AnnotationsActionsModalContent(props: Props): JSX.Element { + function AnnotationsActionsModalContent({ onClose, targetObjectState }: Props): JSX.Element {Update references to
props.onCloseandprops.targetObjectStatewithin the function body to useonCloseandtargetObjectStatedirectly.
297-297: Use destructured props instead of 'props'You are accessing
props.targetObjectStatewhilepropshas been destructured. This leads to inconsistency and can cause confusion.Ensure you are using the destructured
targetObjectStatedirectly:- targetObjectState: props.targetObjectState ?? null, + targetObjectState: targetObjectState ?? null,🧰 Tools
🪛 eslint
[error] 297-297: Must use destructuring props assignment
(react/destructuring-assignment)
381-425: Simplify frame selection logicThe conditional rendering for frame selection could be refactored for better readability.
Consider extracting the frame selection UI into a separate component to make the code more modular and easier to maintain.
cvat-core/src/annotations-collection.ts (3)
165-181: Fix indentation to comply with coding standardsLines 165 to 181 have indentation issues as identified by the linter. Please adjust the indentation to match the project's coding standards.
Apply this diff to fix the indentation:
): { - tags: Tag[]; - shapes: Shape[]; - tracks: Track[]; - } { + tags: Tag[]; + shapes: Shape[]; + tracks: Track[]; + } { const removedCollection: (Shape | Tag | Track)[] = []; const collectionConsistent = [] - .concat(removed.shapes, removed.tags, removed.tracks) - .reduce<boolean>((acc, serializedObject) => { - if (acc && typeof serializedObject.clientID === 'number') { - const collectionObject = this.objects[serializedObject.clientID]; - if (collectionObject) { - removedCollection.push(collectionObject); - return true; - } - } + .concat(removed.shapes, removed.tags, removed.tracks) + .reduce<boolean>((acc, serializedObject) => { + if (acc && typeof serializedObject.clientID === 'number') { + const collectionObject = this.objects[serializedObject.clientID]; + if (collectionObject) { + removedCollection.push(collectionObject); + return true; + } + } return false; - }, true); + }, true);🧰 Tools
🪛 eslint
[error] 165-165: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 166-166: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 167-167: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 168-168: Expected indentation of 8 spaces but found 4.
(@typescript-eslint/indent)
[error] 172-172: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 173-173: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 174-174: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 175-175: Expected indentation of 20 spaces but found 24.
(@typescript-eslint/indent)
[error] 176-176: Expected indentation of 20 spaces but found 24.
(@typescript-eslint/indent)
[error] 177-177: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 178-178: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 180-180: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 181-181: Expected indentation of 8 spaces but found 12.
(@typescript-eslint/indent)
221-221: Split line to adhere to maximum line lengthLine 221 exceeds the maximum allowed line length of 120 characters. Please split the line to improve readability.
Apply this diff to split the line:
- [].concat(removedCollection.map((object) => object.clientID), appendedCollection.map((object) => object.clientID)), + [].concat( + removedCollection.map((object) => object.clientID), + appendedCollection.map((object) => object.clientID), + ),🧰 Tools
🪛 eslint
[error] 221-221: This line has a length of 127. Maximum allowed is 120.
(max-len)
225-225: Remove unnecessaryreturnstatementThe
return;statement at the end of thecommitmethod is unnecessary since it's the last statement in the method. Removing it can clean up the code.Apply this diff to remove the unnecessary return statement:
- return;🧰 Tools
🪛 eslint
[error] 225-225: Unnecessary return statement.
(no-useless-return)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (28)
changelog.d/20241120_234543_sekachev.bs_updated_annotations_actions.md(1 hunks)changelog.d/20241120_234732_sekachev.bs_updated_annotations_actions.md(1 hunks)changelog.d/20241120_234852_sekachev.bs_updated_annotations_actions.md(1 hunks)cvat-core/src/annotations-actions.ts(0 hunks)cvat-core/src/annotations-actions/annotations-actions.ts(1 hunks)cvat-core/src/annotations-actions/base-action.ts(1 hunks)cvat-core/src/annotations-actions/base-collection-action.ts(1 hunks)cvat-core/src/annotations-actions/base-shapes-action.ts(1 hunks)cvat-core/src/annotations-actions/propagate-shapes.ts(1 hunks)cvat-core/src/annotations-actions/remove-filtered-shapes.ts(1 hunks)cvat-core/src/annotations-collection.ts(3 hunks)cvat-core/src/annotations-filter.ts(4 hunks)cvat-core/src/annotations-objects.ts(9 hunks)cvat-core/src/api-implementation.ts(2 hunks)cvat-core/src/api.ts(5 hunks)cvat-core/src/enums.ts(1 hunks)cvat-core/src/index.ts(3 hunks)cvat-core/src/object-state.ts(4 hunks)cvat-core/src/session-implementation.ts(2 hunks)cvat-core/src/session.ts(4 hunks)cvat-ui/src/components/annotation-page/annotations-actions/annotations-actions-modal.tsx(14 hunks)cvat-ui/src/components/annotation-page/annotations-actions/styles.scss(0 hunks)cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item-basics.tsx(3 hunks)cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item-menu.tsx(5 hunks)cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item.tsx(3 hunks)cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item.tsx(4 hunks)cvat-ui/src/cvat-core-wrapper.ts(4 hunks)tests/cypress/e2e/features/annotations_actions.js(0 hunks)
💤 Files with no reviewable changes (3)
- cvat-core/src/annotations-actions.ts
- cvat-ui/src/components/annotation-page/annotations-actions/styles.scss
- tests/cypress/e2e/features/annotations_actions.js
🧰 Additional context used
🪛 LanguageTool
changelog.d/20241120_234543_sekachev.bs_updated_annotations_actions.md
[grammar] ~3-~3: The plural noun “annotations” cannot be used with the article “an”. Did you mean “an annotation” or “annotations”?
Context: ... user may undo or redo changes, made by an annotations action (<https://github.com/cvat-ai/c...
(A_NNS)
🪛 eslint
cvat-core/src/annotations-actions/annotations-actions.ts
[error] 26-26: There should be no space before this paren.
(space-in-parens)
cvat-core/src/annotations-actions/base-action.ts
[error] 36-45: Unnecessary 'else' after 'return'.
(no-else-return)
cvat-core/src/annotations-actions/base-collection-action.ts
[error] 5-5: 'omit' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 107-107: Expected property shorthand.
(object-shorthand)
[error] 139-139: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 140-140: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 141-141: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 143-143: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 144-144: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 145-145: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 147-147: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 148-148: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 149-149: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 151-151: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 152-152: Expected indentation of 8 spaces but found 12.
(@typescript-eslint/indent)
[error] 155-155: Multiple spaces found before 'frameData'.
(no-multi-spaces)
cvat-core/src/annotations-actions/base-shapes-action.ts
[error] 5-5: 'omit' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 79-79: This line has a length of 123. Maximum allowed is 120.
(max-len)
[error] 171-171: Multiple spaces found before 'frameData'.
(no-multi-spaces)
cvat-core/src/annotations-actions/propagate-shapes.ts
[error] 38-38: This line has a length of 136. Maximum allowed is 120.
(max-len)
[error] 39-39: This line has a length of 126. Maximum allowed is 120.
(max-len)
cvat-core/src/annotations-collection.ts
[error] 165-165: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 166-166: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 167-167: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 168-168: Expected indentation of 8 spaces but found 4.
(@typescript-eslint/indent)
[error] 172-172: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 173-173: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 174-174: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 175-175: Expected indentation of 20 spaces but found 24.
(@typescript-eslint/indent)
[error] 176-176: Expected indentation of 20 spaces but found 24.
(@typescript-eslint/indent)
[error] 177-177: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 178-178: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 180-180: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 181-181: Expected indentation of 8 spaces but found 12.
(@typescript-eslint/indent)
[error] 221-221: This line has a length of 127. Maximum allowed is 120.
(max-len)
[error] 225-225: Unnecessary return statement.
(no-useless-return)
cvat-core/src/annotations-filter.ts
[error] 9-9: server-response-types import should occur before import of ./object-state
(import/order)
[error] 10-10: labels import should occur before import of ./object-state
(import/order)
[error] 62-64: Unnecessary 'else' after 'return'.
(no-else-return)
[error] 103-103: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 104-104: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 105-105: Expected indentation of 16 spaces but found 20.
(@typescript-eslint/indent)
[error] 106-106: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 130-130: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 131-131: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 132-132: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 133-133: Expected indentation of 8 spaces but found 4.
(@typescript-eslint/indent)
[error] 137-138: Missing semicolon.
(@typescript-eslint/semi)
[error] 139-139: Missing space after =>.
(arrow-spacing)
[error] 141-141: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 142-142: Expected indentation of 12 spaces but found 16.
(@typescript-eslint/indent)
[error] 143-143: Expected indentation of 8 spaces but found 12.
(@typescript-eslint/indent)
[error] 146-147: Missing trailing comma.
(@typescript-eslint/comma-dangle)
[error] 156-156: 'idx' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 158-158: This line has a length of 127. Maximum allowed is 120.
(max-len)
[error] 165-165: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 174-175: Missing semicolon.
(@typescript-eslint/semi)
[error] 176-176: 'idx' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 182-182: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 193-193: 'idx' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 208-209: Missing semicolon.
(@typescript-eslint/semi)
[error] 226-226: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 227-227: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 228-228: Expected indentation of 12 spaces but found 8.
(@typescript-eslint/indent)
[error] 229-229: Expected indentation of 8 spaces but found 4.
(@typescript-eslint/indent)
[error] 236-236: Unnecessary semicolon.
(@typescript-eslint/no-extra-semi)
cvat-core/src/annotations-objects.ts
[error] 153-153: '_' is defined but never used.
(@typescript-eslint/no-unused-vars)
cvat-core/src/index.ts
[error] 43-44: Missing semicolon.
(@typescript-eslint/semi)
cvat-core/src/session.ts
[error] 176-176: This line has a length of 131. Maximum allowed is 120.
(max-len)
cvat-ui/src/components/annotation-page/annotations-actions/annotations-actions-modal.tsx
[error] 99-99: 'action' is already declared in the upper scope on line 95 column 32.
(@typescript-eslint/no-shadow)
[error] 297-297: Must use destructuring props assignment
(react/destructuring-assignment)
[error] 567-568: Missing semicolon.
(@typescript-eslint/semi)
🪛 GitHub Check: Linter
cvat-core/src/annotations-actions/annotations-actions.ts
[failure] 26-26:
There should be no space before this paren
[failure] 114-114:
Too many blank lines at the end of file. Max of 0 allowed
cvat-core/src/annotations-actions/base-action.ts
[failure] 36-36:
Unnecessary 'else' after 'return'
cvat-core/src/annotations-actions/base-collection-action.ts
[failure] 5-5:
'omit' is defined but never used
[failure] 107-107:
Expected property shorthand
[failure] 139-139:
Expected indentation of 12 spaces but found 16
[failure] 140-140:
Expected indentation of 16 spaces but found 20
[failure] 141-141:
Expected indentation of 12 spaces but found 16
[failure] 143-143:
Expected indentation of 12 spaces but found 16
[failure] 144-144:
Expected indentation of 16 spaces but found 20
🔇 Additional comments (40)
changelog.d/20241120_234852_sekachev.bs_updated_annotations_actions.md (1)
1-4: Verify consistency with related changelog entries
Since this is part of a larger feature set, let's ensure consistency with related changelog entries.
✅ Verification successful
Changelog entries are consistent and complementary
The two changelog entries are related to the same pull request (#8727) and describe complementary features:
- The ability to run annotation actions on specific objects
- The ability to undo/redo changes made by annotation actions
Both entries follow the same format, are properly categorized under "Added", and reference the same PR. The messaging is clear and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related changelog entries to ensure consistent messaging
# Look for other changelog entries related to annotation actions
echo "Related changelog entries:"
fd . changelog.d -t f -e md -x grep -l "annotation.*action" {}
Length of output: 271
Script:
#!/bin/bash
# Let's check the content of both changelog files to verify consistency
echo "Content of changelog entries:"
cat changelog.d/20241120_234852_sekachev.bs_updated_annotations_actions.md
echo "---"
cat changelog.d/20241120_234543_sekachev.bs_updated_annotations_actions.md
Length of output: 476
changelog.d/20241120_234543_sekachev.bs_updated_annotations_actions.md (1)
1-4: LGTM! The changelog entry is well-structured.
The entry correctly documents the new undo/redo feature and follows the proper changelog format with a clear reference to the PR.
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: The plural noun “annotations” cannot be used with the article “an”. Did you mean “an annotation” or “annotations”?
Context: ... user may undo or redo changes, made by an annotations action (<https://github.com/cvat-ai/c...
(A_NNS)
cvat-core/src/annotations-actions/remove-filtered-shapes.ts (2)
1-7: LGTM! Proper license and imports.
The file header contains the correct copyright notice and license. Imports are appropriate for the required functionality.
17-28:
Verify the implementation logic against requirements.
There seems to be a discrepancy between the class name "RemoveFilteredShapes" and its implementation:
- The
run()method marks all shapes as deleted without any filtering - The
applyFilter()method returns the unmodified collection
This might indicate either:
- Missing implementation of the filtering logic
- Incorrect class naming
Let's check if there are any related filter implementations:
Please clarify:
- Should this action apply filtering before removal?
- If not, should the class be renamed to something like
RemoveAllShapes?
✅ Verification successful
Let me gather more context about the filtering implementation in the base class and the annotations filter.
Let me check how the base class and other actions implement filtering to understand the expected behavior.
Based on the implementation details seen in the base class and filter implementations, I can now provide the final response:
Implementation is correct but class name could be clearer
The current implementation in RemoveFilteredShapes is actually correct because:
- The base class
BaseShapesActionalready handles the filtering logic throughAnnotationsFilterbefore calling the action'srunmethod - The action's
applyFiltermethod is correctly implemented as a pass-through since the filtering is already done at the base class level - The
runmethod correctly handles the filtered shapes by marking them for deletion
The class name could be more precise, but it's technically accurate as it operates on shapes that were already filtered by the base implementation.
- Consider renaming to
RemoveShapesfor clarity since the filtering is handled by the base class
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for filter-related implementations in annotation actions
rg -t ts "applyFilter|filter" "cvat-core/src/annotations-actions"
Length of output: 4999
Script:
#!/bin/bash
# Check the base class implementation and annotations filter
ast-grep --pattern 'class BaseShapesAction {
$$$
}'
# Check the AnnotationsFilter implementation
cat cvat-core/src/annotations-filter.ts
Length of output: 9058
Script:
#!/bin/bash
# Check BaseShapesAction implementation
cat cvat-core/src/annotations-actions/base-shapes-action.ts
# Check how other actions implement filtering
ast-grep --pattern 'class $_ extends BaseShapesAction {
$$$
applyFilter($_) {
$$$
}
$$$
}'
Length of output: 7254
cvat-core/src/annotations-actions/annotations-actions.ts (2)
38-39: Verify if all required actions are registered
Currently, only RemoveFilteredShapes and PropagateShapes are registered. Based on the PR objectives mentioning "Enhanced running of annotations actions", verify if additional actions need to be registered.
✅ Verification successful
Based on the search results, I can now generate a final response:
The action registrations are complete and correct
The codebase search reveals that only two concrete action implementations exist that extend the action hierarchy:
RemoveFilteredShapes(extendsBaseShapesAction)PropagateShapes(extendsBaseCollectionAction)
Both of these actions are properly registered in the code. There are no other concrete action implementations that need to be registered.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for other action implementations that might need registration
# Search for other action class implementations
ast-grep --pattern 'class $_ extends BaseAction {
$$$
}'
Length of output: 60
Script:
#!/bin/bash
# Let's try a different approach to find action classes
# Search for files containing "Action" in their name
fd "Action" --type f
# Search for class definitions that might be actions
rg "class.*Action" -A 2
# Look for action registrations
rg "registerAction" -A 1
Length of output: 6124
16-17: Verify the implementation of imported actions
The imported actions should implement their respective base classes correctly.
✅ Verification successful
Imports are correctly implemented and follow the inheritance hierarchy
The verification confirms that:
RemoveFilteredShapesproperly extendsBaseShapesActionPropagateShapesproperly extendsBaseCollectionAction- Both
BaseShapesActionandBaseCollectionActionextend the abstractBaseActionclass - All classes implement the required abstract methods from their parent classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that RemoveFilteredShapes and PropagateShapes properly extend BaseAction
# Check RemoveFilteredShapes implementation
ast-grep --pattern 'class RemoveFilteredShapes extends BaseAction {
$$$
}'
# Check PropagateShapes implementation
ast-grep --pattern 'class PropagateShapes extends BaseAction {
$$$
}'
Length of output: 152
Script:
#!/bin/bash
# Let's try a different approach to verify the implementations
# First, let's check if these files exist
fd "remove-filtered-shapes.ts|propagate-shapes.ts" cvat-core/src/annotations-actions/
# Then, let's examine their content
echo "=== Content of remove-filtered-shapes.ts ==="
rg -A 10 "class RemoveFilteredShapes" cvat-core/src/annotations-actions/
echo "=== Content of propagate-shapes.ts ==="
rg -A 10 "class PropagateShapes" cvat-core/src/annotations-actions/
# Let's also check for the BaseAction definition
echo "=== BaseAction definition ==="
rg -A 5 "class BaseAction" cvat-core/src/annotations-actions/
Length of output: 3275
Script:
#!/bin/bash
# Let's verify the inheritance chain by checking for BaseShapesAction and BaseCollectionAction
echo "=== BaseShapesAction definition ==="
rg -A 5 "class BaseShapesAction" cvat-core/src/annotations-actions/
echo "=== BaseCollectionAction definition ==="
rg -A 5 "class BaseCollectionAction" cvat-core/src/annotations-actions/
# Let's also check if these base classes extend BaseAction
echo "=== Inheritance chain ==="
rg "class Base(Shapes|Collection)Action extends" cvat-core/src/annotations-actions/
Length of output: 1981
cvat-ui/src/cvat-core-wrapper.ts (1)
29-30: LGTM! Consistent import/export declarations.
The new types and classes are properly imported and exported, maintaining consistency throughout the module. This ensures that the enhanced annotation functionality is properly exposed to other parts of the application.
Also applies to: 74-75, 96-98
cvat-core/src/enums.ts (2)
151-151: LGTM! The new enum value follows existing conventions.
The addition of COMMIT_ANNOTATIONS to HistoryActions follows the established naming patterns and integrates well with the existing history tracking system.
151-151: Verify the usage of the new history action.
Let's ensure this new enum value is properly utilized across the codebase.
✅ Verification successful
Let me check the actual implementation to verify proper usage of this enum.
The new COMMIT_ANNOTATIONS enum is properly integrated and used in the codebase
The verification shows that:
- The enum is correctly used in
cvat-core/src/annotations-collection.tsas part of the history actions implementation - It's properly integrated with the annotations commit functionality exposed through the session interface
- The implementation follows the existing pattern for history actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of the new COMMIT_ANNOTATIONS history action
# Search for direct string references
echo "Searching for direct string references to 'Commit annotations'..."
rg -l "Commit annotations"
# Search for enum usage
echo "Searching for COMMIT_ANNOTATIONS enum usage..."
rg -l "HistoryActions.COMMIT_ANNOTATIONS"
# Search for potential history-related files that might need updates
echo "Searching for history-related files..."
fd -e ts -e tsx -e js -e jsx history
Length of output: 546
Script:
#!/bin/bash
# Check the implementation in annotations-collection.ts
echo "Checking COMMIT_ANNOTATIONS usage in annotations-collection.ts:"
rg -A 5 "COMMIT_ANNOTATIONS" annotations-collection.ts
# Check history-related implementation
echo -e "\nChecking history implementation in annotations-history.ts:"
ast-grep --pattern 'class HistoryActions {
$$$
}'
Length of output: 444
Script:
#!/bin/bash
# Check the implementation in the correct path
echo "Checking COMMIT_ANNOTATIONS usage in cvat-core:"
rg -A 5 "COMMIT_ANNOTATIONS" cvat-core/src/annotations-collection.ts
# Check history implementation
echo -e "\nChecking history implementation:"
rg -A 10 "class .*History" cvat-core/src/annotations-history.ts
# Check for any commit-related actions
echo -e "\nChecking for commit-related patterns:"
rg -A 5 "commit.*annotation" cvat-core/src/
Length of output: 1494
cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item-basics.tsx (2)
87-87: LGTM!
The props destructuring follows the existing pattern and maintains good code organization.
Line range hint 182-182: Verify memoization effectiveness with the new action
With the addition of runAnnotationAction, verify that the React.memo wrapper is still effectively preventing unnecessary re-renders. Consider adding a useCallback wrapper if the action is defined in the parent component.
✅ Verification successful
Based on the search results, I can now generate a final response as we have enough context about runAnnotationAction. The function is defined in a class component (object-item.tsx) and passed down through props, not created inline in any functional component.
Memoization is correctly implemented for this component
The runAnnotationAction is defined as a class method using the class fields syntax, which means it has a stable reference and won't cause unnecessary re-renders of the memoized ObjectItemBasics component. Since it's not created inline or with hooks in a functional component, there's no need for useCallback. The current implementation maintains proper memoization effectiveness.
- The function is defined in
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item.tsxas a class method - It's passed down through props to
ObjectItemBasicswhich is correctly wrapped inReact.memo - The stable reference of the class method ensures the memoization won't break
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for parent component implementations to verify callback handling
ast-grep --pattern 'const $runAnnotationAction = useCallback($$$)'
Length of output: 68
Script:
#!/bin/bash
# Let's first find where runAnnotationAction is defined and used
rg "runAnnotationAction" -A 2 -B 2
# Also search for the ObjectItemBasics component definition and its props
ast-grep --pattern 'interface $Props {
$$$
}'
# And check the actual component implementation
ast-grep --pattern 'const ObjectItemBasics = memo<$_>(($_) => {
$$$
})'
Length of output: 7065
cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item.tsx (2)
77-77: LGTM!
The props destructuring is correctly updated to include the new method.
138-138: Verify ItemBasics component implementation.
Ensure that the ItemBasics component's Props interface includes the runAnnotationAction prop and handles it appropriately.
✅ Verification successful
The search results show that runAnnotationAction is properly declared in the object-item-basics.tsx interface and is being used within the component. Let me verify the complete implementation to ensure it's handled correctly.
The runAnnotationAction prop is properly implemented
The verification confirms that:
- The
runAnnotationActionprop is correctly declared in the Props interface ofobject-item-basics.tsx - It's properly passed through the component hierarchy
- The prop is being used appropriately in the component's implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if ItemBasics component properly declares and handles the runAnnotationAction prop
# Look for the Props interface in object-item-basics.tsx
ast-grep --pattern 'interface Props {
$$$
runAnnotationAction: () => void;
$$$
}'
# Look for usage of runAnnotationAction in object-item-basics.tsx
rg "runAnnotationAction" --type ts
Length of output: 1629
Script:
#!/bin/bash
# Check the complete implementation of ItemBasics component
rg -A 5 "interface Props" cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item-basics.tsx
# Check how runAnnotationAction is used in the component
rg -B 2 -A 2 "runAnnotationAction" cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item-basics.tsx
Length of output: 811
cvat-core/src/index.ts (2)
175-176: Verify backward compatibility of action interface changes
The interface changes from runActions to runAction and the addition of callAction might impact existing code.
Let's verify the usage of these methods in the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response:
Interface changes are safe and maintain backward compatibility
The changes from runActions to runAction and addition of callAction are part of a new action handling system that has been properly implemented throughout the codebase. The search results show:
- No existing usage of
runActionswas found, indicating this wasn't a breaking change - The new
runActionandcallActionmethods are properly integrated in the action system:- Defined in
annotations-actions/annotations-actions.ts - Used consistently through the action system implementation
- Have proper type definitions and implementations
- Defined in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usage of runActions to ensure all calls are updated
rg "runActions" --type ts
# Search for existing action method calls to understand impact
rg "\.(run|call)Action" --type ts
Length of output: 61
Script:
#!/bin/bash
# Let's try a broader search for action-related patterns
rg -i "action" --type ts -A 2 cvat-core/src/
# Also search for run and call patterns
rg "\.(run|call)\(" --type ts -A 2 cvat-core/src/
# Let's check the git history for these files to understand the changes
git diff HEAD~1 cvat-core/src/index.ts
Length of output: 92042
220-221: Verify implementation of new action base classes
The addition of BaseShapesAction and BaseCollectionAction suggests a more modular approach to action handling. Let's ensure they are properly implemented and documented.
✅ Verification successful
Let me gather more information about the base classes and their usage.
Base action classes are properly implemented and used
The verification shows that both BaseShapesAction and BaseCollectionAction are well-implemented abstract classes with:
- Clear interfaces defining input/output types
- Abstract methods
run()andapplyFilter() - Proper inheritance from
BaseAction - Active usage in concrete implementations like
RemoveFilteredShapesandPropagateShapes
The modular approach is correctly implemented with type safety and consistent patterns across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation of base classes
ast-grep --pattern 'class BaseShapesAction {
$$$
}'
ast-grep --pattern 'class BaseCollectionAction {
$$$
}'
# Check for documentation
rg -A 5 "class (BaseShapesAction|BaseCollectionAction)" --type ts
Length of output: 1467
Script:
#!/bin/bash
# Check for the parent BaseAction class implementation
ast-grep --pattern 'class BaseAction {
$$$
}'
# Check for implementations/usage of these base classes
rg -l "extends (BaseShapesAction|BaseCollectionAction)" --type ts
# Check for types used in these classes
rg "(CollectionActionInput|ShapesActionInput|CollectionActionOutput|ShapesActionOutput)" --type ts -A 2
Length of output: 5400
cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item-menu.tsx (2)
347-352: LGTM!
The menu integration is well-implemented, following the existing patterns for conditional rendering based on the readonly state.
Line range hint 237-352: Verify integration with annotation actions system.
Since this is part of a larger enhancement to the annotation actions system:
- Ensure proper integration with the new action handling mechanisms
- Add unit tests for the new RunAnnotationActionItem component
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item.tsx (3)
8-8: LGTM: Modern React root API usage
The import of createRoot follows React 18's recommended way of creating roots for rendering components.
447-447: LGTM: Proper prop passing
The runAnnotationAction method is correctly passed as a prop to the ObjectStateItemComponent.
24-24: Verify modal component implementation and usage
Let's verify the consistency of modal implementations across the codebase.
✅ Verification successful
Let me try a different approach with modified commands to search for the modal implementations and usages.
Let me check the actual implementation and usage of the modal component to make a final determination.
Based on the review of the implementation and usage, I can now provide a final response:
Modal implementation is consistent and properly managed
The AnnotationsActionsModalContent component is a well-structured React modal that:
- Uses Ant Design's Modal component consistently
- Properly handles mounting/unmounting through React's createRoot
- Manages its own state and cleanup
- Is used appropriately in the object-item.tsx file for annotation actions
The implementation follows the application's patterns for modal dialogs and there are no issues with duplicate implementations or inconsistent usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other modal implementations and usages
# Check for other modal implementations
echo "Checking for other modal implementations..."
rg -l "createElement.*modal" --type tsx
# Check for AnnotationsActionsModalContent usage
echo "Checking AnnotationsActionsModalContent usage..."
rg -l "AnnotationsActionsModalContent" --type tsx
# Check for similar patterns of dynamic modal creation
echo "Checking for similar modal creation patterns..."
ast-grep --pattern 'createRoot($root).render(<$_/>)'
Length of output: 511
Script:
#!/bin/bash
# Search for modal implementations and usages without file type restrictions
echo "Checking for modal implementations..."
rg -l "createElement.*[Mm]odal"
echo "Checking AnnotationsActionsModalContent usage..."
rg -l "AnnotationsActionsModalContent"
echo "Checking for modal-related components..."
rg -l "Modal.*Content"
# Check the specific file content
echo "Checking the specific file content..."
cat "cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item.tsx"
Length of output: 16566
Script:
#!/bin/bash
# Check the implementation of AnnotationsActionsModalContent
echo "Checking AnnotationsActionsModalContent implementation..."
cat "cvat-ui/src/components/annotation-page/annotations-actions/annotations-actions-modal.tsx"
# Check for similar modal creation patterns with context
echo "Checking similar modal creation patterns..."
rg "createRoot.*render.*Modal" -A 5
Length of output: 25700
cvat-core/src/object-state.ts (2)
2-2: LGTM: Copyright and imports updated appropriately
The copyright has been extended to 2024, and necessary serialized types have been imported to support the new export functionality.
Also applies to: 11-11
Line range hint 550-563: LGTM: Delete implementation rename maintains functionality
The rename to deleteImplementation is consistent with other changes while preserving the existing argument validation and functionality.
cvat-core/src/api-implementation.ts (2)
42-44: Verify the impact of import changes on existing code.
The change from runActions to runAction suggests a shift from batch processing to individual action processing. This could potentially break existing code that relies on the old behavior.
Let's verify the usage of these functions in the codebase:
59-60: Ensure proper error handling in the new action implementations.
The addition of callAction and update to use runAction introduces new API methods. These should include proper error handling and maintain backward compatibility.
Let's verify the implementation details:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Error handling is properly implemented in the action system architecture
The implementation follows a robust error handling pattern through:
- Async/await usage in all action-related functions (
runAction,callAction) - Type-safe implementation with proper interfaces and abstract classes
- Action-specific error handling through the class inheritance system (BaseAction -> BaseShapesAction)
- Progress and cancellation callback support for long-running operations
The architecture ensures errors are properly propagated up the call stack where they can be handled by the UI layer's extensive try-catch blocks, as evidenced by the numerous error handlers found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in the new action implementations
# Test: Look for try-catch blocks and error handling in the implementation
ast-grep --pattern 'try {
$$$
} catch ($$$) {
$$$
}' src/annotations-actions/annotations-actions.ts
# Test: Check for backward compatibility handling
rg -A 5 'runAction|callAction' src/annotations-actions/annotations-actions.ts
Length of output: 366
Script:
#!/bin/bash
# Let's first find the correct location of these action implementations
fd -t f "action" --exec rg -l "runAction|callAction"
# Then check their implementations for error handling
fd -t f "action" --exec ast-grep --pattern 'try {
$$$
} catch ($$$) {
$$$
}'
# Also search for the implementation details
rg -A 5 "implementationMixin.*(?:run|call)Action"
Length of output: 118011
Script:
#!/bin/bash
# Let's check the implementation of runAction and callAction
rg -A 10 "function runAction|function callAction" cvat-core/src/
# Also check for error handling patterns in these implementations
ast-grep --pattern 'try {
$$$
runAction($$$)
$$$
} catch ($$$) {
$$$
}'
ast-grep --pattern 'try {
$$$
callAction($$$)
$$$
} catch ($$$) {
$$$
}'
Length of output: 2205
Script:
#!/bin/bash
# Let's examine the implementation details of these action functions
rg -B 5 -A 20 "export async function runAction|export async function callAction" cvat-core/src/annotations-actions/annotations-actions.ts
# Also check for error handling in the implementation of BaseShapesAction
rg -A 10 "class BaseShapesAction" cvat-core/src/annotations-actions/
Length of output: 2488
Script:
#!/bin/bash
# Let's check the implementation of runShapesAction and callShapesAction which handle the actual execution
rg -B 2 -A 15 "async function runShapesAction|async function callShapesAction" cvat-core/src/annotations-actions/
# Also check for error handling in the actual action implementations
rg -A 10 "extends BaseShapesAction" cvat-core/src/annotations-actions/
Length of output: 1160
cvat-core/src/session.ts (2)
369-373: LGTM! Well-typed commit method declaration
The type declaration is well-structured, using appropriate TypeScript features like Omit utility type to handle the SerializedCollection parameters.
444-444: LGTM! Proper method binding
The commit method is correctly bound to the instance context, following the established pattern in the codebase.
cvat-core/src/annotations-objects.ts (4)
527-536: LGTM! Well-structured context extension
The implementation properly extends the parent context and adds type-safe method bindings for save and export.
846-855: LGTM! Consistent with Shape class implementation
The implementation maintains consistency with the Shape class pattern while providing track-specific context.
1424-1433: LGTM! Maintains implementation consistency
The implementation follows the established pattern, ensuring consistency across annotation types.
600-600: LGTM! Consistent internal context implementation
The __internal property is consistently implemented across all annotation types, providing uniform access to context methods.
Also applies to: 950-950, 1481-1481, 2052-2052, 3094-3094
cvat-core/src/annotations-actions/base-action.ts (3)
16-20: Type 'ActionParameters' is properly structured
The ActionParameters type is well-defined, allowing for flexible parameter definitions with appropriate types and default values.
22-31: Abstract class 'BaseAction' is well-defined
The BaseAction class correctly defines the required abstract methods and getters, ensuring that subclasses will implement the necessary functionality.
39-39: Verify compatibility of 'Object.hasOwn' with target environments
The method Object.hasOwn is used, which is supported in ES2022 and later. Ensure that your project's target environment or transpilation settings support this method, or consider using Object.prototype.hasOwnProperty.call for broader compatibility.
Run the following script to verify the project's target ECMAScript version in tsconfig.json:
Also applies to: 41-41
✅ Verification successful
Object.hasOwn usage is compatible with target environments
The codebase is configured with target: "ESNext" in all tsconfig files and has browser targets specified in package.json that include Chrome >= 63 and Firefox > 102. Object.hasOwn is fully supported in these environments (Chrome 93+ and Firefox 92+), making its usage appropriate and safe in this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the target ECMAScript version in tsconfig.json
# Test: Ensure the target is ES2022 or later.
# Expected result: The target version is 'ES2022' or newer.
# Search for tsconfig.json and extract the target version
fd -p -t f tsconfig.json --exec jq '.compilerOptions.target' {}
Length of output: 224
Script:
#!/bin/bash
# Let's verify the actual tsconfig.json content and browser targets in package.json
echo "=== TSConfig files ==="
fd tsconfig.json -x cat {}
echo -e "\n=== Package.json browserslist or targets ==="
fd package.json -x cat {} | grep -A 5 -B 5 "browserslist\|targets"
Length of output: 3807
cvat-core/src/api.ts (5)
24-26: New imports added for action classes
The imports for BaseAction, BaseCollectionAction, and BaseShapesAction are correctly added.
196-196: Updated actions.register method to accept BaseAction
The method signature change broadens applicability and aligns with the new action handling mechanism.
202-203: Simplified actions.run method signature
The run method now accepts a single BaseAction, simplifying its usage.
226-249: Added new actions.call method
The call method enhances functionality by allowing execution of actions with specific states and frames.
449-450: Registered new action classes in classes
BaseShapesAction and BaseCollectionAction are appropriately added to the classes section.
cvat-ui/src/components/annotation-page/annotations-actions/annotations-actions-modal.tsx (2)
544-548: Ensure proper function binding and context
When defining updateProgressWrapper, ensure that it maintains the correct context if used in asynchronous operations.
Please confirm that updateProgressWrapper operates correctly in all contexts where it's called, especially in async operations.
122-122: 🛠️ Refactor suggestion
Rename variable to prevent shadowing 'action'
In the reducer case SET_ACTIVE_ANNOTATIONS_ACTION, the action parameter is shadowed by the reducer's action argument. This can cause confusion and potential issues.
Apply this diff:
- if (action.type === ReducerActionType.SET_ACTIVE_ANNOTATIONS_ACTION) {
- const { activeAction } = action.payload;
+ if (action.type === ReducerActionType.SET_ACTIVE_ANNOTATIONS_ACTION) {
+ const { activeAction } = action.payload;Alternatively, consider renaming the reducer's parameter to avoid shadowing.
Likely invalid or redundant comment.
changelog.d/20241120_234732_sekachev.bs_updated_annotations_actions.md
Outdated
Show resolved
Hide resolved
| export async function callAction( | ||
| instance: Job | Task, | ||
| action: BaseAction, | ||
| actionParameters: Record<string, string>, | ||
| frame: number, | ||
| states: ObjectState[], | ||
| onProgress: (message: string, progress: number) => void, | ||
| cancelled: () => boolean, | ||
| ): Promise<void> { | ||
| if (action instanceof BaseShapesAction) { | ||
| return callShapesAction( | ||
| instance, | ||
| action, | ||
| actionParameters, | ||
| frame, | ||
| states, | ||
| onProgress, | ||
| cancelled, | ||
| ); | ||
| } | ||
|
|
||
| if (action instanceof BaseCollectionAction) { | ||
| return callCollectionAction( | ||
| instance, | ||
| action, | ||
| actionParameters, | ||
| frame, | ||
| states, | ||
| onProgress, | ||
| cancelled, | ||
| ); | ||
| } | ||
|
|
||
| return Promise.resolve(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent error handling improvements
Similar to runAction, this implementation would benefit from improved error handling and type safety.
Apply similar improvements:
export async function callAction(
instance: Job | Task,
action: BaseAction,
actionParameters: Record<string, string>,
frame: number,
states: ObjectState[],
onProgress: (message: string, progress: number) => void,
cancelled: () => boolean,
): Promise<void> {
+ if (!action) {
+ throw new ArgumentError('Action must be provided');
+ }
+
+ if (!Array.isArray(states)) {
+ throw new ArgumentError('States must be an array');
+ }
+
if (action instanceof BaseShapesAction) {
return callShapesAction(
instance,
action,
actionParameters,
frame,
states,
onProgress,
cancelled,
);
}
if (action instanceof BaseCollectionAction) {
return callCollectionAction(
instance,
action,
actionParameters,
frame,
states,
onProgress,
cancelled,
);
}
- return Promise.resolve();
+ throw new ArgumentError(`Unsupported action type: ${action.constructor.name}`);
}📝 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.
| export async function callAction( | |
| instance: Job | Task, | |
| action: BaseAction, | |
| actionParameters: Record<string, string>, | |
| frame: number, | |
| states: ObjectState[], | |
| onProgress: (message: string, progress: number) => void, | |
| cancelled: () => boolean, | |
| ): Promise<void> { | |
| if (action instanceof BaseShapesAction) { | |
| return callShapesAction( | |
| instance, | |
| action, | |
| actionParameters, | |
| frame, | |
| states, | |
| onProgress, | |
| cancelled, | |
| ); | |
| } | |
| if (action instanceof BaseCollectionAction) { | |
| return callCollectionAction( | |
| instance, | |
| action, | |
| actionParameters, | |
| frame, | |
| states, | |
| onProgress, | |
| cancelled, | |
| ); | |
| } | |
| return Promise.resolve(); | |
| } | |
| export async function callAction( | |
| instance: Job | Task, | |
| action: BaseAction, | |
| actionParameters: Record<string, string>, | |
| frame: number, | |
| states: ObjectState[], | |
| onProgress: (message: string, progress: number) => void, | |
| cancelled: () => boolean, | |
| ): Promise<void> { | |
| if (!action) { | |
| throw new ArgumentError('Action must be provided'); | |
| } | |
| if (!Array.isArray(states)) { | |
| throw new ArgumentError('States must be an array'); | |
| } | |
| if (action instanceof BaseShapesAction) { | |
| return callShapesAction( | |
| instance, | |
| action, | |
| actionParameters, | |
| frame, | |
| states, | |
| onProgress, | |
| cancelled, | |
| ); | |
| } | |
| if (action instanceof BaseCollectionAction) { | |
| return callCollectionAction( | |
| instance, | |
| action, | |
| actionParameters, | |
| frame, | |
| states, | |
| onProgress, | |
| cancelled, | |
| ); | |
| } | |
| throw new ArgumentError(`Unsupported action type: ${action.constructor.name}`); | |
| } |
| function RunAnnotationActionItem(props: ItemProps): JSX.Element { | ||
| const { toolProps } = props; | ||
| const { runAnnotationAction } = toolProps; | ||
| return ( | ||
| <Button | ||
| type='link' | ||
| icon={<FunctionOutlined />} | ||
| onClick={runAnnotationAction} | ||
| className='cvat-object-item-menu-remove-object' | ||
| > | ||
| Run annotation action | ||
| </Button> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix className and add tooltip for consistency.
The component has a few inconsistencies with other menu items:
- Using incorrect className 'cvat-object-item-menu-remove-object'
- Missing tooltip unlike other action buttons
function RunAnnotationActionItem(props: ItemProps): JSX.Element {
const { toolProps } = props;
const { runAnnotationAction } = toolProps;
return (
+ <CVATTooltip title='Run an annotation action on this object'>
<Button
type='link'
icon={<FunctionOutlined />}
onClick={runAnnotationAction}
- className='cvat-object-item-menu-remove-object'
+ className='cvat-object-item-menu-run-action'
>
Run annotation action
</Button>
+ </CVATTooltip>
);
}📝 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.
| function RunAnnotationActionItem(props: ItemProps): JSX.Element { | |
| const { toolProps } = props; | |
| const { runAnnotationAction } = toolProps; | |
| return ( | |
| <Button | |
| type='link' | |
| icon={<FunctionOutlined />} | |
| onClick={runAnnotationAction} | |
| className='cvat-object-item-menu-remove-object' | |
| > | |
| Run annotation action | |
| </Button> | |
| ); | |
| } | |
| function RunAnnotationActionItem(props: ItemProps): JSX.Element { | |
| const { toolProps } = props; | |
| const { runAnnotationAction } = toolProps; | |
| return ( | |
| <CVATTooltip title='Run an annotation action on this object'> | |
| <Button | |
| type='link' | |
| icon={<FunctionOutlined />} | |
| onClick={runAnnotationAction} | |
| className='cvat-object-item-menu-run-action' | |
| > | |
| Run annotation action | |
| </Button> | |
| </CVATTooltip> | |
| ); | |
| } |
| private runAnnotationAction = (): void => { | ||
| const { objectState } = this.props; | ||
| const div = window.document.createElement('div'); | ||
| window.document.body.append(div); | ||
| const root = createRoot(div); | ||
| root.render( | ||
| <AnnotationsActionsModalContent | ||
| targetObjectState={objectState} | ||
| onClose={() => { | ||
| root.unmount(); | ||
| div.remove(); | ||
| }} | ||
| />, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider component lifecycle and error handling improvements
The modal implementation has several areas for improvement:
- Add cleanup on component unmount to prevent memory leaks
- Add error handling for DOM operations
- Consider accessibility implications
Consider this improved implementation:
private runAnnotationAction = (): void => {
const { objectState } = this.props;
+ try {
const div = window.document.createElement('div');
+ div.setAttribute('role', 'dialog');
+ div.setAttribute('aria-modal', 'true');
window.document.body.append(div);
const root = createRoot(div);
+ const cleanup = () => {
+ root.unmount();
+ div.remove();
+ };
+
+ // Ensure cleanup on component unmount
+ this.modalCleanup = cleanup;
root.render(
<AnnotationsActionsModalContent
targetObjectState={objectState}
- onClose={() => {
- root.unmount();
- div.remove();
- }}
+ onClose={cleanup}
/>,
);
+ } catch (error) {
+ console.error('Failed to create annotation action modal:', error);
};
};
+public componentWillUnmount(): void {
+ if (this.modalCleanup) {
+ this.modalCleanup();
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
cvat-core/src/annotations-filter.ts
Outdated
| import { SerializedCollection } from 'server-response-types'; | ||
| import { Attribute, Label } from 'labels'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust import order to comply with conventions
The imports from 'server-response-types' and 'labels' should occur before local imports like './object-state' to maintain the import order convention.
Apply this diff to reorder the imports:
import jsonLogic from 'json-logic-js';
+import { SerializedCollection } from 'server-response-types';
+import { Attribute, Label } from 'labels';
import { SerializedData } from './object-state';
import { AttributeType, ObjectType, ShapeType } from './enums';
-import { SerializedCollection } from 'server-response-types';
-import { Attribute, Label } from 'labels';📝 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.
| import { SerializedCollection } from 'server-response-types'; | |
| import { Attribute, Label } from 'labels'; | |
| import jsonLogic from 'json-logic-js'; | |
| import { SerializedCollection } from 'server-response-types'; | |
| import { Attribute, Label } from 'labels'; | |
| import { SerializedData } from './object-state'; | |
| import { AttributeType, ObjectType, ShapeType } from './enums'; |
🧰 Tools
🪛 eslint
[error] 9-9: server-response-types import should occur before import of ./object-state
(import/order)
[error] 10-10: labels import should occur before import of ./object-state
(import/order)
| ) | ||
|
|
There was a problem hiding this comment.
Add missing semicolon after function call
There's a missing semicolon after the function call, which can cause syntax errors.
Apply this diff:
- )
+ );📝 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.
| ) | |
| ); | |
🧰 Tools
🪛 eslint
[error] 567-568: Missing semicolon.
(@typescript-eslint/semi)
| const reducer = (state: State, action: ActionUnion<typeof reducerActions>): State => { | ||
| if (action.type === ReducerActionType.SET_ANNOTATIONS_ACTIONS) { | ||
| const { actions } = action.payload; | ||
| const list = state.targetObjectState ? actions | ||
| .filter((action) => action.isApplicableForObject(state.targetObjectState as ObjectState)) : actions; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename variable to prevent shadowing 'action'
The variable action in the .filter callback shadows the action variable from the outer scope of the reducer. This can lead to confusion and potential bugs.
Apply this diff to rename the inner variable:
- .filter((action) => action.isApplicableForObject(state.targetObjectState as ObjectState)) : actions;
+ .filter((item) => item.isApplicableForObject(state.targetObjectState as ObjectState)) : actions;📝 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.
| .filter((action) => action.isApplicableForObject(state.targetObjectState as ObjectState)) : actions; | |
| .filter((item) => item.isApplicableForObject(state.targetObjectState as ObjectState)) : actions; |
🧰 Tools
🪛 eslint
[error] 99-99: 'action' is already declared in the upper scope on line 95 column 32.
(@typescript-eslint/no-shadow)
| @@ -100,19 +91,42 @@ | |||
| ), | |||
| }; | |||
|
|
|||
| let lastSelectedActions: [string, Record<string, string>][] = []; | |||
There was a problem hiding this comment.
Avoid using module-level mutable variables for component state
The variable lastSelectedActions is declared outside of the component and mutated within the reducer. This can lead to unexpected behavior, especially if multiple instances of this component are rendered or if the component is re-mounted.
Consider using the useRef hook to store mutable data that persists across renders without causing re-renders, or integrate lastSelectedActions into the component's state using useState.
- let lastSelectedActions: [string, Record<string, string>][] = [];
+ const lastSelectedActionsRef = useRef<[string, Record<string, string>][]>([]);Update references to lastSelectedActions in your code to use lastSelectedActionsRef.current.
Committable suggestion skipped: line range outside the PR's diff.
bsekachev
changed the title
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8727 +/- ##
===========================================
- Coverage 74.20% 74.08% -0.12%
===========================================
Files 401 406 +5
Lines 43504 43713 +209
Branches 3950 3984 +34
===========================================
+ Hits 32280 32386 +106
- Misses 11224 11327 +103
|
Motivation and context
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
Release Notes
New Features
commitmethod for managing annotation changes in theCollectionclass.runAnnotationActionmethod across various components for improved action handling.Bug Fixes
Documentation
Style