Cannot set properties of undefined (setting 'serverID') #7882
Conversation
|
Warning Rate Limit Exceeded@bsekachev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 50 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes in Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Track
participant SkeletonTrack
User->>Track: Modify shape
Track->>Track: appendShapeActionToHistory(actionType, frame, undoShape, redoShape, undoSource, redoSource, undoFrame, redoFrame)
Track->>SkeletonTrack: Update frame and shape
SkeletonTrack->>Track: Confirmation of update
User->>Track: Save track
Track->>User: Track saved successfully
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat-core/src/annotations-objects.ts (7 hunks)
Additional comments not posted (1)
cvat-core/src/annotations-objects.ts (1)
Line range hint
1135-1164: The addition ofundoFrameandredoFrameparameters toappendShapeActionToHistoryenhances the method's functionality by allowing frame-specific undo and redo actions. This is a positive change for maintaining a detailed history of shape modifications across different frames.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- changelog.d/20240513_125509_boris_fixed_tracks.md (1 hunks)
Additional Context Used
LanguageTool (1)
changelog.d/20240513_125509_boris_fixed_tracks.md (1)
Near line 3: Before the countable noun ‘save’ an article or a possessive pronoun is necessary.
Context: ...ting 'serverID')"** when trying to save job after removing the first keyframe of a ...
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
bsekachev
changed the title
bsekachev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (9)
cvat-core/src/object-state.ts (4)
Line range hint
230-235: Consider usingfor...ofinstead offorEachfor better readability and performance in modern JavaScript environments.- data.elements.forEach((element: ObjectState) => { - element.hidden = hidden; - }); + for (const element of data.elements) { + element.hidden = hidden; + }
Line range hint
245-245: Optimize the mapping and flattening of points by using.flatMap()instead of.map().flat().- return data.elements.map((element) => element.points).flat(); + return data.elements.flatMap((element) => element.points);
Line range hint
257-260: Use template literals for string concatenation to improve readability and maintainability.- 'Points are expected to be an array of numbers but got ' + typeof points === 'object' ? points.constructor.name : typeof points + `Points are expected to be an array of numbers but got ${typeof points === 'object' ? points.constructor.name : typeof points}` - 'Tried to set wrong number of points for a skeleton' + `(${points.length} vs ${currentPoints.length}})` + `Tried to set wrong number of points for a skeleton (${points.length} vs ${currentPoints.length})` - 'Attributes are expected to be an object but got ' + typeof attributes === 'object' ? attributes.constructor.name : typeof attributes + `Attributes are expected to be an object but got ${typeof attributes === 'object' ? attributes.constructor.name : typeof attributes}`Also applies to: 268-269, 386-391
Line range hint
490-490: Use optional chaining to safely access properties.- if (serialized.__internal) { + if (serialized.__internal?) { - if (this.__internal && this.__internal.delete) { + if (this.__internal?.delete) {Also applies to: 501-501
cvat-core/src/annotations-objects.ts (5)
Line range hint
322-323: Use template literals for string operations to improve readability and performance.- 'No one left position or right position was found. ' + 'Interpolation impossible. Client ID: ' + this.clientID + `No one left position or right position was found. Interpolation impossible. Client ID: ${this.clientID}`
Line range hint
1436-1437: Use template literals for string operations to improve readability and performance.- 'An unexpected type of shape "' + type + '"' + `An unexpected type of shape "${type}"`
Line range hint
70-70: Specify explicit types instead of usinganyto enhance type safety and code maintainability.Consider defining a more specific type or interface that accurately represents the expected structure of the data.
Also applies to: 146-146, 333-333, 1414-1414
Line range hint
2633-2633: Remove unnecessary return statements in functions declared with a void return type.- return;Also applies to: 2637-2637, 2716-2716
Line range hint
2681-2690: Remove unnecessaryelseclause aftercontinuestatement to simplify control flow.- else if (y2 <= y) { - if (position(x1, y1, x2, y2) < 0) { - wn--; - } - } + else if (y2 <= y && position(x1, y1, x2, y2) < 0) { + wn--; + }
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- changelog.d/20240513_125509_boris_fixed_tracks.md (1 hunks)
- cvat-core/src/annotations-objects.ts (19 hunks)
- cvat-core/src/object-state.ts (7 hunks)
Additional Context Used
Biome (27)
cvat-core/src/annotations-objects.ts (20)
70-70: Unexpected any. Specify a different type.
146-146: Unexpected any. Specify a different type.
322-323: Template literals are preferred over string concatenation.
333-333: Unexpected any. Specify a different type.
451-451: Prefer for...of instead of forEach.
1414-1414: Unexpected any. Specify a different type.
1436-1437: Template literals are preferred over string concatenation.
2172-2172: Prefer for...of instead of forEach.
2173-2173: Prefer for...of instead of forEach.
2174-2174: Prefer for...of instead of forEach.
2181-2181: Prefer for...of instead of forEach.
2186-2186: Prefer for...of instead of forEach.
2191-2191: Prefer for...of instead of forEach.
2633-2633: The function should not return a value because its return type is void.
2637-2637: The function should not return a value because its return type is void.
2681-2681: Unnecessary continue statement
2682-2690: This else clause can be omitted because previous branches break early.
2716-2716: The function should not return a value because its return type is void.
2900-2903: Prefer for...of instead of forEach.
3037-3039: Prefer for...of instead of forEach.
cvat-core/src/object-state.ts (7)
233-235: Prefer for...of instead of forEach.
245-245: The call chain .map().flat() can be replaced with a single .flatMap() call.
257-260: Template literals are preferred over string concatenation.
268-269: Template literals are preferred over string concatenation.
386-391: Template literals are preferred over string concatenation.
490-490: Change to an optional chain.
501-501: Change to an optional chain.
Additional comments not posted (2)
changelog.d/20240513_125509_boris_fixed_tracks.md (1)
3-4: The changelog entry is clear and effectively communicates the fix related to the 'serverID' issue.cvat-core/src/annotations-objects.ts (1)
1135-1144: RefactorappendShapeActionToHistoryto include new parameters for better history management.This change allows tracking frame changes alongside shape actions, which enhances the undo/redo functionality by providing more context about the state changes.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (4)
cvat-core/src/object-state.ts (4)
Line range hint
230-235: Prefer usingfor...ofinstead offorEachfor better readability and performance.- data.elements.forEach((element: ObjectState) => { - element.hidden = hidden; - }); + for (const element of data.elements) { + element.hidden = hidden; + }
Line range hint
245-245: Use.flatMap()instead of.map().flat()for more efficient array flattening.- return data.elements.map((element) => element.points).flat(); + return data.elements.flatMap((element) => element.points);
Line range hint
257-260: Use template literals for string concatenation to improve readability and maintainability.- 'Points are expected to be an array of numbers ' + - `but got ${ - typeof points === 'object' ? points.constructor.name : typeof points - }`, + `Points are expected to be an array of numbers but got ${typeof points === 'object' ? points.constructor.name : typeof points}`, - 'Tried to set wrong number of points for a skeleton' + - `(${points.length} vs ${currentPoints.length}})`, + `Tried to set wrong number of points for a skeleton (${points.length} vs ${currentPoints.length})`, - 'Attributes are expected to be an object ' + - `but got ${ - typeof attributes === 'object' ? - attributes.constructor.name : - typeof attributes - }`, + `Attributes are expected to be an object but got ${typeof attributes === 'object' ? attributes.constructor.name : typeof attributes}`,Also applies to: 268-269, 385-390
Line range hint
489-489: Consider using optional chaining to safely access properties.- if (serialized.__internal) { + if (serialized.__internal?) { - if (this.__internal && this.__internal.delete) { + if (this.__internal?.delete) {Also applies to: 500-500
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- cvat-core/src/annotations-objects.ts (31 hunks)
- cvat-core/src/object-state.ts (6 hunks)
Additional Context Used
Biome (27)
cvat-core/src/annotations-objects.ts (20)
85-85: Unexpected any. Specify a different type.
161-161: Unexpected any. Specify a different type.
337-338: Template literals are preferred over string concatenation.
466-466: Prefer for...of instead of forEach.
1421-1421: Unexpected any. Specify a different type.
1443-1444: Template literals are preferred over string concatenation.
2180-2180: Prefer for...of instead of forEach.
2181-2181: Prefer for...of instead of forEach.
2182-2182: Prefer for...of instead of forEach.
2189-2189: Prefer for...of instead of forEach.
2194-2194: Prefer for...of instead of forEach.
2199-2199: Prefer for...of instead of forEach.
2641-2641: The function should not return a value because its return type is void.
2645-2645: The function should not return a value because its return type is void.
2689-2689: Unnecessary continue statement
2690-2698: This else clause can be omitted because previous branches break early.
2724-2724: The function should not return a value because its return type is void.
2908-2911: Prefer for...of instead of forEach.
3045-3047: Prefer for...of instead of forEach.
3055-3065: Prefer for...of instead of forEach.
cvat-core/src/object-state.ts (7)
233-235: Prefer for...of instead of forEach.
245-245: The call chain .map().flat() can be replaced with a single .flatMap() call.
257-260: Template literals are preferred over string concatenation.
268-269: Template literals are preferred over string concatenation.
385-390: Template literals are preferred over string concatenation.
489-489: Change to an optional chain.
500-500: Change to an optional chain.
Additional comments not posted (2)
cvat-core/src/annotations-objects.ts (2)
836-836: Verify thatserverTrackedShapeToClienthandles all necessary fields.Verification successful
The function
serverTrackedShapeToClienthas been located and its implementation analyzed. It appears to handle the necessary fields fromSerializedTrack['shapes'][0]as follows:
idis mapped toserverIDoccludedis directly mappedz_orderis mapped tozOrderpointsis directly mappedoutsideis directly mappedrotationis mapped with a default value of0if not presentattributesare processed and accumulated into an objectBased on this analysis, it seems that
serverTrackedShapeToClienthandles all the necessary fields fromSerializedTrack['shapes'][0].
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `serverTrackedShapeToClient` handles all necessary fields. # Test: Search for the implementation of `serverTrackedShapeToClient`. Expect: All fields from `SerializedTrack['shapes'][0]` are handled. ast-grep --lang typescript --pattern $'function serverTrackedShapeToClient($_) { $$$ }'Length of output: 88
Script:
#!/bin/bash # Description: Locate the implementation of `serverTrackedShapeToClient` and verify it handles all necessary fields. # Step 1: Locate the implementation of `serverTrackedShapeToClient`. rg 'function serverTrackedShapeToClient' -t typescript # Step 2: Once located, extract the function implementation to analyze its handling of fields. # Note: The following command will be executed only if the function is found in the previous step. rg -A 20 'function serverTrackedShapeToClient' -t typescriptLength of output: 189
Script:
#!/bin/bash # Description: Locate the implementation of `serverTrackedShapeToClient` and verify it handles all necessary fields. # Step 1: Find all TypeScript files. fd -e ts # Step 2: Search for the implementation of `serverTrackedShapeToClient` within the found TypeScript files. fd -e ts -x rg 'function serverTrackedShapeToClient' {} # Step 3: Once located, extract the function implementation to analyze its handling of fields. # Note: The following command will be executed only if the function is found in the previous step. fd -e ts -x rg -A 20 'function serverTrackedShapeToClient' {}Length of output: 7578
348-348: Ensure proper error handling when updating server IDs.
| protected appendShapeActionToHistory( | ||
| actionType, | ||
| frame, | ||
| undoShape, | ||
| redoShape, | ||
| undoSource, | ||
| redoSource, | ||
| undoFrame, | ||
| redoFrame, | ||
| ): void { |
There was a problem hiding this comment.
Refactor to improve readability of appendShapeActionToHistory.
- this.history.do(
- actionType,
- () => {
- if (!undoShape) {
- delete this.shapes[frame];
- } else {
- this.shapes[frame] = undoShape;
- }
- this.source = undoSource;
- this.frame = undoFrame;
- this.updated = Date.now();
- },
- () => {
- if (!redoShape) {
- delete this.shapes[frame];
- } else {
- this.shapes[frame] = redoShape;
- }
- this.source = redoSource;
- this.frame = redoFrame;
- this.updated = Date.now();
- },
- [this.clientID],
- frame,
- );
+ this.history.do(
+ actionType,
+ this.createHistoryCallback(undoShape, undoSource, undoFrame),
+ this.createHistoryCallback(redoShape, redoSource, redoFrame),
+ [this.clientID],
+ frame,
+ );
+
+ // Helper method to create a history callback
+ protected createHistoryCallback(shape, source, frame) {
+ return () => {
+ if (!shape) {
+ delete this.shapes[frame];
+ } else {
+ this.shapes[frame] = shape;
+ }
+ this.source = source;
+ this.frame = frame;
+ this.updated = Date.now();
+ };
+ }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.
| protected appendShapeActionToHistory( | |
| actionType, | |
| frame, | |
| undoShape, | |
| redoShape, | |
| undoSource, | |
| redoSource, | |
| undoFrame, | |
| redoFrame, | |
| ): void { | |
| protected appendShapeActionToHistory( | |
| actionType, | |
| frame, | |
| undoShape, | |
| redoShape, | |
| undoSource, | |
| redoSource, | |
| undoFrame, | |
| redoFrame, | |
| ): void { | |
| this.history.do( | |
| actionType, | |
| this.createHistoryCallback(undoShape, undoSource, undoFrame), | |
| this.createHistoryCallback(redoShape, redoSource, redoFrame), | |
| [this.clientID], | |
| frame, | |
| ); | |
| } | |
| // Helper method to create a history callback | |
| protected createHistoryCallback(shape, source, frame) { | |
| return () => { | |
| if (!shape) { | |
| delete this.shapes[frame]; | |
| } else { | |
| this.shapes[frame] = shape; | |
| } | |
| this.source = source; | |
| this.frame = frame; | |
| this.updated = Date.now(); | |
| }; | |
| } |
| result.shapes.forEach((shape) => { | ||
| shape.points = []; | ||
| }); |
There was a problem hiding this comment.
Refactor to use for...of loop for better performance and readability.
- result.shapes.forEach((shape) => {
- shape.points = [];
- });
+ for (const shape of result.shapes) {
+ shape.points = [];
+ }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.
| result.shapes.forEach((shape) => { | |
| shape.points = []; | |
| }); | |
| for (const shape of result.shapes) { | |
| shape.points = []; | |
| } |
| Object.values(this.shapes).forEach((shape) => { | ||
| // skeletons do not have their own points | ||
| shape.points = []; | ||
| }); |
There was a problem hiding this comment.
Refactor to use for...of loop for better performance and readability.
- Object.values(this.shapes).forEach((shape) => {
- shape.points = [];
- });
+ for (const shape of Object.values(this.shapes)) {
+ shape.points = [];
+ }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.
| Object.values(this.shapes).forEach((shape) => { | |
| // skeletons do not have their own points | |
| shape.points = []; | |
| }); | |
| for (const shape of Object.values(this.shapes)) { | |
| // skeletons do not have their own points | |
| shape.points = []; | |
| } |
|
I want to find another way, but this PR still contains lots of useful ideas I will apply later. |
Motivation and context
Resolved #7879
Motivation:
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