Clear some client-side events collected by analytics #8304
Conversation
bsekachev
requested review from
mdacoca,
Marishka17 and
nmanovic
as code owners
|
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 streamlines the client event system by removing several outdated events, including Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant UI
Client->>Server: Request to perform action (e.g., upload annotations)
alt Event Supported
Server->>Client: Acknowledge event
else Event Deprecated
Server->>Client: Return deprecation notice
end
Client->>UI: Update UI based on server response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (4)
cvat-canvas/README.md (1)
75-77: Clarify expected outputs forcanvas.dragshapeandcanvas.resizeshape.The removal of return type annotations for these events might lead to ambiguity. Consider providing a brief description of the expected outputs to aid developers in understanding these events.
- - canvas.dragshape + - canvas.dragshape // Expected output: { id: number } (if applicable) - - canvas.resizeshape + - canvas.resizeshape // Expected output: { id: number } (if applicable)cvat-ui/src/components/annotation-page/attribute-annotation-workspace/attribute-annotation-sidebar/attribute-annotation-sidebar.tsx (1)
2-2: Logging functionality related tojobInstanceis still present. ThejobInstanceproperty is still used in various parts of the codebase, including logging actions. This suggests that the changes might not fully align with the new logging strategy or requirements.
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx: Contains multiple instances ofjobInstance.logger.log(...).cvat-ui/src/actions/annotation-actions.ts: Contains instances ofjobInstance.logger.log(...).Please review these occurrences to ensure alignment with the new logging strategy.
Analysis chain
Verify changes to the logging mechanism.
The
jobInstanceproperty and related logging functionality have been removed. Ensure that this change aligns with the new logging strategy or requirements.Also applies to: 122-122
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of logging functionality related to jobInstance. # Test: Search for any remaining logging related to jobInstance in the codebase. Expect: No direct logging for these actions. rg --type js 'logger\.log.*jobInstance'Length of output: 41
Script:
#!/bin/bash # Description: Search for any remaining references to jobInstance in the codebase. # Test: Look for any occurrences of jobInstance to ensure all related functionality is removed. rg 'jobInstance'Length of output: 39883
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (1)
760-762: Ensure logging sufficiency forresizeObject.The logging for
resizeObjectin theonCanvasShapeResizedmethod does not include specific identifiers or additional context. This might result in insufficient logging for debugging or auditing purposes. Consider enhancing the log details to include relevant identifiers or context to ensure comprehensive logging.
- Location:
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx, line 760Analysis chain
Ensure logging sufficiency for
resizeObject.The logging call for
resizeObjectno longer includes specific identifiers. Verify that this meets the logging requirements and no essential information is omitted.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the logging for `resizeObject` is sufficient. # Test: Search for logging calls related to `resizeObject` to ensure no critical information is lost. rg --type tsx 'logger\.log\(EventScope\.resizeObject' -A 3Length of output: 88
Script:
#!/bin/bash # Description: Verify that the logging for `resizeObject` is sufficient. # Define `tsx` as a recognized file type for ripgrep rg --type-add 'tsx:*.{ts,tsx}' --type tsx 'logger\.log\(EventScope\.resizeObject' -A 3Length of output: 521
cvat-ui/src/actions/annotation-actions.ts (1)
892-892: Enhance Job Loading Logging for ComprehensivenessThe current logging for job loading captures only the duration. Consider reviewing and potentially enhancing the logging to include additional information such as job ID, status, or any errors encountered during the loading process to ensure comprehensiveness.
cvat-ui/src/actions/annotation-actions.ts: Line withawait job.logger.log(EventScope.loadJob, { duration: Date.now() - start });Analysis chain
Ensure comprehensive logging for job loading.
The logging for job loading now focuses on duration. Verify that this meets the logging requirements and no essential information is omitted.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the logging for job loading is comprehensive. # Test: Search for logging calls related to job loading to ensure no critical information is lost. rg --type ts 'logger\.log\(EventScope\.loadJob' -A 3Length of output: 375
Script:
#!/bin/bash # Search for all logging statements in the annotation-actions.ts file to check for additional job loading logs rg --type ts 'logger\.log' cvat-ui/src/actions/annotation-actions.ts -A 3Length of output: 1620
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- changelog.d/20240814_151947_boris_update_events.md (1 hunks)
- changelog.d/20240814_153835_boris_update_events.md (1 hunks)
- cvat-canvas/README.md (1 hunks)
- cvat-canvas/package.json (1 hunks)
- cvat-canvas/src/typescript/canvasView.ts (2 hunks)
- cvat-core/package.json (1 hunks)
- cvat-core/src/enums.ts (2 hunks)
- cvat-core/src/logger.ts (2 hunks)
- cvat-ui/package.json (1 hunks)
- cvat-ui/src/actions/annotation-actions.ts (4 hunks)
- cvat-ui/src/actions/boundaries-actions.ts (2 hunks)
- cvat-ui/src/actions/import-actions.ts (2 hunks)
- cvat-ui/src/components/annotation-page/attribute-annotation-workspace/attribute-annotation-sidebar/attribute-annotation-sidebar.tsx (7 hunks)
- cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (3 hunks)
- cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-buttons.tsx (2 hunks)
- cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item-details.tsx (3 hunks)
- cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item.tsx (2 hunks)
- cvat-ui/src/utils/event-recorder.ts (2 hunks)
- cvat/apps/events/serializers.py (1 hunks)
- site/content/en/docs/administration/advanced/analytics.md (1 hunks)
Files skipped from review due to trivial changes (7)
- changelog.d/20240814_151947_boris_update_events.md
- changelog.d/20240814_153835_boris_update_events.md
- cvat-canvas/package.json
- cvat-core/package.json
- cvat-ui/package.json
- cvat-ui/src/actions/boundaries-actions.ts
- cvat-ui/src/actions/import-actions.ts
Additional context used
LanguageTool
site/content/en/docs/administration/advanced/analytics.md
[uncategorized] ~136-~136: Loose punctuation mark.
Context: ...ent events: -load:cvat-load:job,save:job-send:exception- `draw...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~140-~140: Loose punctuation mark.
Context: ...ob-send:exception-draw:object,paste:object,copy:object,propaga...(UNLIKELY_OPENING_PUNCTUATION)
Additional comments not posted (14)
cvat-ui/src/utils/event-recorder.ts (1)
32-37: Enhance logging with additional environment details.The added properties (
screenWidth,screenHeight,language,hardwareConcurrency,deviceMemory,jsHeapSizeLimit) provide valuable context for debugging and analytics. The use oflodashfor safe property access is a good practice to prevent runtime errors.cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item-details.tsx (2)
11-11: UseThunkDispatchfor improved type safety.Switching to
ThunkDispatchenhances type safety and clarity when dispatching asynchronous actions. This aligns with best practices for using Redux Thunk.
80-80: Consider implications of removing logging inchangeAttribute.The removal of the logging functionality in
changeAttributemay simplify the component but could impact traceability of attribute changes. Ensure that this change aligns with the overall logging strategy.cvat/apps/events/serializers.py (1)
32-36: Streamline allowed event scopes.The modification of
ALLOWED_SCOPESreflects a shift in focus or functionality. Ensure that the removal of these scopes aligns with the application's requirements and does not impact necessary functionality. Consider updating documentation to reflect these changes.cvat-core/src/enums.ts (1)
Line range hint
74-74:
Verify the impact of removed events on the codebase.The removal of constants like
restoreJob,uploadAnnotations,sendTaskInfo,lockObject,changeAttribute, andchangeLabelmay affect other parts of the codebase that rely on these events. Ensure that all references to these events have been updated or removed.Verification successful
Removed EventScope constants are not used elsewhere in the codebase.
The removed constants
restoreJob,uploadAnnotations,sendTaskInfo,lockObject,changeAttribute, andchangeLabelfrom theEventScopeenum are not referenced in any.tsfiles. This indicates that their removal does not affect other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that removed EventScope constants are not used elsewhere in the codebase. # Test: Search for the removed constants. Expect: No occurrences. rg --type typescript $'EventScope.restoreJob|EventScope.uploadAnnotations|EventScope.sendTaskInfo|EventScope.lockObject|EventScope.changeAttribute|EventScope.changeLabel'Length of output: 206
Script:
#!/bin/bash # Description: Verify that removed EventScope constants are not used elsewhere in the codebase. # Test: Search for the removed constants in .ts files. Expect: No occurrences. rg 'EventScope.restoreJob|EventScope.uploadAnnotations|EventScope.sendTaskInfo|EventScope.lockObject|EventScope.changeAttribute|EventScope.changeLabel' --glob '*.ts'Length of output: 167
cvat-core/src/logger.ts (1)
32-32: Verify the impact of removingEventScope.changeAttributefromIgnoredRules.Ensure that the removal of
EventScope.changeAttributefromIgnoredRulesdoes not affect any existing logging functionality that might rely on this event being ignored.Verification successful
Removal of
EventScope.changeAttributeis safe and has no impact.The search confirmed that
EventScope.changeAttributeis not used elsewhere in the codebase. Therefore, its removal fromIgnoredRulesdoes not affect any existing logging functionality.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the removal of `EventScope.changeAttribute` does not affect logging functionality. # Test: Search for the usage of `EventScope.changeAttribute` in the codebase. Expect: No occurrences. rg --type typescript $'EventScope.changeAttribute'Length of output: 84
Script:
#!/bin/bash # Description: Verify that the removal of `EventScope.changeAttribute` does not affect logging functionality. # Test: Search for the usage of `EventScope.changeAttribute` in the codebase. Expect: No occurrences. rg 'EventScope.changeAttribute'Length of output: 31
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-buttons.tsx (1)
116-118: Verify changes to the logging mechanism.The logging functionality for
lockandunlockactions has been removed. Ensure that this change aligns with the new logging strategy or requirements.Also applies to: 124-126
cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/object-item.tsx (1)
323-325: Verify changes to the logging mechanism.The logging functionality for label changes has been removed. Ensure that this change aligns with the new logging strategy or requirements.
site/content/en/docs/administration/advanced/analytics.md (1)
136-136: Remove redundant punctuation.The bullet points for client events should not start with a hyphen when listed in a markdown format.
- - `load:cvat` + `load:cvat`Likely invalid or redundant comment.
Tools
LanguageTool
[uncategorized] ~136-~136: Loose punctuation mark.
Context: ...ent events: -load:cvat-load:job,save:job-send:exception- `draw...(UNLIKELY_OPENING_PUNCTUATION)
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (2)
755-757: Ensure logging sufficiency fordragObject.The logging call for
dragObjectno longer includes specific identifiers. Verify that this meets the logging requirements and no essential information is omitted.
667-669: Simplify logging while ensuring it meets requirements.The logging for
drawObjectandpasteObjectevents has been simplified by removing detailed properties. Ensure that this change aligns with the application's logging requirements and that no critical information is lost.cvat-ui/src/actions/annotation-actions.ts (1)
813-813: Simplify logging forrotateImage.The logging for
rotateImagehas been simplified by removing the angle parameter. Ensure that this change aligns with the application's logging requirements and that no critical information is lost.Verification successful
Simplified logging for
rotateImageis acceptable.The logging simplification for
rotateImagedoes not appear to affect other parts of the codebase, as there are no additional references toEventScope.rotateImage. Thus, no critical information is lost by removing the angle parameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the simplified logging for `rotateImage` is sufficient. # Test: Search for logging calls related to `rotateImage` to ensure no critical information is lost. rg --type ts 'logger\.log\(EventScope\.rotateImage' -A 3Length of output: 334
Script:
#!/bin/bash # Search for all references to EventScope.rotateImage to understand its usage and significance. rg 'EventScope\.rotateImage' -A 3Length of output: 311
cvat-canvas/src/typescript/canvasView.ts (2)
Line range hint
1456-1456:
Simplification acknowledged: Verify event handler updates.The removal of the
detailproperty from thecanvas.resizeshapeevent simplifies the event structure. Ensure that any event handlers relying on this detail are updated accordingly.Verification successful
Event handlers updated: No reliance on removed
detailproperty found.The search did not reveal any event handlers for
canvas.resizeshapethat rely on the removeddetailproperty, indicating that the handlers have been updated or do not require this property. This supports the simplification of the event structure.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the event handlers for `canvas.resizeshape` are updated to not rely on the removed `detail` property. # Test: Search for event handlers for `canvas.resizeshape`. Expect: No usage of the removed `detail` property. rg --type javascript --type typescript -A 5 $'canvas.resizeshape'Length of output: 99
Script:
#!/bin/bash # Description: Verify if the event handlers for `canvas.resizeshape` are updated to not rely on the removed `detail` property. # Corrected Test: Search for event handlers for `canvas.resizeshape`. Expect: No usage of the removed `detail` property. rg --glob '*.js' --glob '*.ts' -A 5 $'canvas.resizeshape'Length of output: 549
Line range hint
1104-1104:
Simplification acknowledged: Verify event handler updates.The removal of the
detailproperty from thecanvas.dragshapeevent simplifies the event structure. Ensure that any event handlers relying on this detail are updated accordingly.
| - `send:task_info` | ||
|
|
||
| - `draw:object`, `paste:object`, `copy:object`, `propagate:object`, `drag:object`, `resize:object`, `delete:object`, `lock:object`, `merge:objects`, `split:objects`, `group:objects`, `slice:object`, | ||
| - `draw:object`, `paste:object`, `copy:object`, `propagate:object`, `drag:object`, `resize:object`, `delete:object`, `merge:objects`, `split:objects`, `group:objects`, `slice:object`, |
There was a problem hiding this comment.
Remove redundant punctuation.
The bullet points for client events should not start with a hyphen when listed in a markdown format.
- - `draw:object`, `paste:object`, `copy:object`, `propagate:object`, `drag:object`, `resize:object`, `delete:object`, `merge:objects`, `split:objects`, `group:objects`, `slice:object`,
+ `draw:object`, `paste:object`, `copy:object`, `propagate:object`, `drag:object`, `resize:object`, `delete:object`, `merge:objects`, `split:objects`, `group:objects`, `slice:object`,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.
| - `draw:object`, `paste:object`, `copy:object`, `propagate:object`, `drag:object`, `resize:object`, `delete:object`, `merge:objects`, `split:objects`, `group:objects`, `slice:object`, | |
| `draw:object`, `paste:object`, `copy:object`, `propagate:object`, `drag:object`, `resize:object`, `delete:object`, `merge:objects`, `split:objects`, `group:objects`, `slice:object`, |
Tools
LanguageTool
[uncategorized] ~140-~140: Loose punctuation mark.
Context: ...ob-send:exception-draw:object,paste:object,copy:object,propaga...(UNLIKELY_OPENING_PUNCTUATION)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8304 +/- ##
========================================
Coverage 83.33% 83.33%
========================================
Files 391 391
Lines 41566 41563 -3
Branches 3839 3838 -1
========================================
- Hits 34639 34637 -2
+ Misses 6927 6926 -1
|
|
Motivation and context
dragandresizeeventszoomeventsResolved #7994
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
cvat-canvasdocumentation for better readability.