fix(quality-insights): added improved event display #879
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces an Internal Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Component as UI Component
participant Telemetry as TelemetryManager
participant EventEmitter as EventEmitter
User->>Component: trigger action
Component->>Telemetry: sendEvent({ eventType: InternalEventType.ACTION_EVENT, metadata: { event, node } })
Telemetry->>EventEmitter: emit(InternalEventType.ACTION_EVENT, { metadata })
Component->>Component: continue original handler
sequenceDiagram
autonumber
participant Solution as Solution Init (e.g., FileUploader)
participant Telemetry as TelemetryManager
participant EventEmitter as EventEmitter
Solution->>Telemetry: sendEvent({ eventType: InternalEventType.INIT_SOLUTION })
Telemetry->>EventEmitter: emit(InternalEventType.INIT_SOLUTION, payload?)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![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
🧹 Nitpick comments (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
68-70: Consider stronger typing for metadata structure.The current
Record<string, unknown>type for metadata is flexible but loses type safety. Based on usage across the codebase, a more specific type could help catch errors:{ event: string; node: string; [key: string]: unknown; }However, the current implementation is acceptable if maximum flexibility is desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/abstract/managers/TelemetryManager.ts(1 hunks)src/blocks/CameraSource/CameraSource.ts(3 hunks)src/blocks/FileItem/FileItem.ts(3 hunks)src/blocks/UploadCtxProvider/EventEmitter.ts(2 hunks)src/blocks/UploadList/UploadList.ts(2 hunks)src/blocks/UrlSource/UrlSource.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/blocks/CameraSource/CameraSource.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventType(14-38)
src/blocks/FileItem/FileItem.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventType(14-38)
src/blocks/UploadList/UploadList.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventType(14-38)
src/blocks/UrlSource/UrlSource.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventType(14-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
src/blocks/UrlSource/UrlSource.ts (1)
29-34: LGTM! Event emission follows the established pattern.The ACTION_EVENT is properly emitted with consistent metadata structure before the upload logic executes.
src/blocks/UploadList/UploadList.ts (2)
41-46: LGTM! Consistent event emission pattern.The ACTION_EVENT emission follows the established pattern and provides proper tracking for the add-more action.
59-64: LGTM! Consistent event emission pattern.The ACTION_EVENT emission follows the established pattern and provides proper tracking for the clear-all action.
src/blocks/CameraSource/CameraSource.ts (4)
179-186: LGTM! Consistent event emission pattern.The ACTION_EVENT emission for camera shot action follows the established pattern.
192-199: LGTM! Consistent event emission pattern.The ACTION_EVENT emission for start camera action follows the established pattern.
210-217: LGTM! Consistent event emission pattern.The ACTION_EVENT emission for retake action follows the established pattern.
220-227: LGTM! Consistent event emission pattern.The ACTION_EVENT emission for accept action follows the established pattern.
src/blocks/FileItem/FileItem.ts (2)
88-93: LGTM! Consistent event emission pattern.The ACTION_EVENT emission for edit action follows the established pattern and properly uses the EventType constant.
101-106: LGTM! Consistent event emission pattern.The ACTION_EVENT emission for remove action follows the established pattern and properly uses the EventType constant.
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
11-11: LGTM! New event type properly defined.The ACTION_EVENT is correctly added to InternalEventType and will be available via the spread in EventType.
egordidenko
force-pushed
the
feat/quality-insights
branch
from
4316723 to
5bc7a80
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/blocks/CameraSource/CameraSource.ts (1)
178-227: Consistent ACTION_EVENT implementation looks good.The four handlers (
onShot,onStartCamera,onRetake,onAccept) follow a consistent pattern of emitting ACTION_EVENT before executing their core actions. This enhances observability for user interactions.Optional improvement: Consider wrapping the
emitcalls in try-catch blocks to prevent potential emit failures from blocking the user actions:onShot: () => { + try { this.emit(EventType.ACTION_EVENT, { metadata: { event: 'shot-camera', node: this.tagName, }, }); + } catch (error) { + console.error('Failed to emit ACTION_EVENT', error); + } this._shot(); },Apply the same pattern to the other three handlers if you choose to implement this safeguard.
Note: Events are emitted before actions complete, which tracks user intent rather than successful completion. This appears intentional for telemetry purposes but means failed actions will still be recorded as initiated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/abstract/managers/TelemetryManager.ts(1 hunks)src/blocks/CameraSource/CameraSource.ts(3 hunks)src/blocks/FileItem/FileItem.ts(3 hunks)src/blocks/UploadCtxProvider/EventEmitter.ts(2 hunks)src/blocks/UploadList/UploadList.ts(2 hunks)src/blocks/UrlSource/UrlSource.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/blocks/UploadList/UploadList.ts
- src/blocks/FileItem/FileItem.ts
- src/blocks/UploadCtxProvider/EventEmitter.ts
- src/blocks/UrlSource/UrlSource.ts
- src/abstract/managers/TelemetryManager.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/blocks/CameraSource/CameraSource.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventType(14-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/blocks/CameraSource/CameraSource.ts (1)
8-8: ACTION_EVENT is properly defined and the import is correct.Verification confirms that
ACTION_EVENTis defined insrc/blocks/UploadCtxProvider/EventEmitter.ts(line 11) with its corresponding type signature (lines 68-70). The import inCameraSource.tsand all usages across the codebase are consistent with this definition.
egordidenko
force-pushed
the
feat/quality-insights
branch
from
5bc7a80 to
9351afa
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors telemetry event tracking to use a centralized event emission pattern. Instead of directly calling telemetryManager.sendEvent() with custom payloads, the code now emits ACTION_EVENT events through the standard emit() method, which automatically forwards them to the telemetry manager.
Key changes:
- Introduced a new
ACTION_EVENTevent type in theEventEmitterfor tracking user actions - Replaced direct
telemetryManager.sendEvent()calls withemit(EventType.ACTION_EVENT, ...)across multiple components - Updated
sendEventCloudImageEditor()to use the new event emission pattern - Removed
FILE_URL_CHANGEDandGROUP_CREATEDfrom telemetry exclusion list
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/blocks/UploadCtxProvider/EventEmitter.ts | Added ACTION_EVENT event type and its payload type definition |
| src/abstract/managers/TelemetryManager.ts | Updated sendEventCloudImageEditor() to emit ACTION_EVENT and removed two events from exclusion list |
| src/blocks/FileItem/FileItem.ts | Replaced direct telemetry calls with emit(ACTION_EVENT) for edit and remove actions |
| src/blocks/UrlSource/UrlSource.ts | Added ACTION_EVENT emission for upload-from-url action |
| src/blocks/UploadList/UploadList.ts | Added ACTION_EVENT emissions for add-more and clear-all actions |
| src/blocks/CameraSource/CameraSource.ts | Added ACTION_EVENT emissions for camera-related actions (shot, start, retake, accept) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EventType.FILE_UPLOAD_START, | ||
| EventType.FILE_UPLOAD_PROGRESS, | ||
| EventType.FILE_UPLOAD_SUCCESS, | ||
| EventType.FILE_UPLOAD_FAILED, |
There was a problem hiding this comment.
Removing FILE_URL_CHANGED and GROUP_CREATED from the excluded events list will cause these events to be sent to telemetry. However, these events are still defined in EventPayload and may still be emitted elsewhere in the codebase. This change could result in telemetry data being sent for events that were previously excluded, which may not be intentional. If these events should now be tracked, verify that this behavioral change is documented and expected.
| EventType.FILE_UPLOAD_FAILED, | |
| EventType.FILE_UPLOAD_FAILED, | |
| EventType.FILE_URL_CHANGED, | |
| EventType.GROUP_CREATED, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
68-70: Consider stronger typing for ACTION_EVENT metadata (optional).The
Record<string, unknown>type for metadata is very flexible but loses type safety. If the metadata structures become standardized across components, consider defining a union type of specific shapes:type ActionEventMetadata = | { event: 'shot-camera' | 'start-camera' | 'retake-camera' | 'accept-camera'; node: string } | { event: 'edit-file' | 'remove-file'; node: string } | { event: 'add-more' | 'clear-all'; node: string } | { event: 'upload-from-url'; node: string } | { tabId: string; node: string | undefined; event: string; [key: string]: unknown }; [EventType.ACTION_EVENT]: { metadata: ActionEventMetadata; };This would provide better autocomplete and catch mismatches at compile time, but the current loose typing is acceptable if flexibility is preferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/abstract/managers/TelemetryManager.ts(1 hunks)src/blocks/CameraSource/CameraSource.ts(3 hunks)src/blocks/FileItem/FileItem.ts(3 hunks)src/blocks/UploadCtxProvider/EventEmitter.ts(2 hunks)src/blocks/UploadList/UploadList.ts(2 hunks)src/blocks/UrlSource/UrlSource.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/blocks/FileItem/FileItem.ts
- src/blocks/UrlSource/UrlSource.ts
- src/blocks/UploadList/UploadList.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/blocks/CameraSource/CameraSource.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventType(14-38)
src/abstract/managers/TelemetryManager.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventType(14-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: build
🔇 Additional comments (6)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
11-11: LGTM!The ACTION_EVENT addition to InternalEventType follows the existing pattern and is properly defined.
src/abstract/managers/TelemetryManager.ts (2)
86-104: Verify the removal of FILE_URL_CHANGED and GROUP_CREATED from excluded events is intentional.According to the AI summary,
FILE_URL_CHANGEDandGROUP_CREATEDwere removed from the_excludedEventslist. This means these events will now be processed and sent as telemetry, which could:
- Increase telemetry volume and associated costs
- Provide better observability into file URL changes and group creation events
Please confirm this change aligns with your telemetry strategy and monitoring requirements.
147-154: ****The behavior change is not breaking. When
sendEventCloudImageEditor()callsthis._block.emit(EventType.ACTION_EVENT, ...), the Block'semit()method automatically invokesthis.telemetryManager.sendEvent()with the event type and payload. Telemetry is still sent—the refactoring simply routes it through the established event system rather than callingsendEvent()directly. The method name remains appropriate, and the handler is built into the Block's emit mechanism.Likely an incorrect or invalid review comment.
src/blocks/CameraSource/CameraSource.ts (3)
8-8: LGTM!The EventType import is correctly added to support ACTION_EVENT emissions.
178-186: LGTM!The ACTION_EVENT emission before the camera shot action provides good observability. The metadata structure is consistent and includes both the action type and component context.
191-227: LGTM - Consistent ACTION_EVENT pattern across all camera actions!The implementation of ACTION_EVENT emissions for
onStartCamera,onRetake, andonAcceptfollows the same clean pattern asonShot:
- Events are emitted before the action executes (good for tracking user intent)
- Metadata consistently includes both the specific action name and component context
- The pattern is maintainable and provides good observability across all camera interactions
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import { throttle } from '../../utils/throttle'; | ||
| import { ExternalUploadSource } from '../../utils/UploadSource'; | ||
| import './file-item.css'; | ||
| import { EventType, InternalEventType } from '../UploadCtxProvider/EventEmitter'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
The best way to fix the problem is to remove the unused import EventType from line 16 in src/blocks/FileItem/FileItem.ts, ensuring you only keep the InternalEventType import (assuming it is used somewhere else). Specifically, you should update line 16 to only import InternalEventType from '../UploadCtxProvider/EventEmitter' and remove the unused symbol. No other code needs to be changed, and no imports need to be added.
| @@ -13,7 +13,7 @@ | ||
| import { throttle } from '../../utils/throttle'; | ||
| import { ExternalUploadSource } from '../../utils/UploadSource'; | ||
| import './file-item.css'; | ||
| import { EventType, InternalEventType } from '../UploadCtxProvider/EventEmitter'; | ||
| import { InternalEventType } from '../UploadCtxProvider/EventEmitter'; | ||
| import { FileItemConfig } from './FileItemConfig'; | ||
|
|
||
| const FileItemState = Object.freeze({ |
| import { ActivityBlock } from '../../abstract/ActivityBlock'; | ||
| import { UploaderBlock } from '../../abstract/UploaderBlock'; | ||
| import { UploadSource } from '../../utils/UploadSource'; | ||
| import { EventType, InternalEventType } from '../UploadCtxProvider/EventEmitter'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the problem, we should remove the unused import EventType from the import statement on line 5 in src/blocks/UrlSource/UrlSource.ts. Only InternalEventType should be imported from '../UploadCtxProvider/EventEmitter', as it is the only identifier used in the file. The change should be made directly to the relevant import statement, without affecting any other part of the code.
| @@ -2,7 +2,7 @@ | ||
| import { ActivityBlock } from '../../abstract/ActivityBlock'; | ||
| import { UploaderBlock } from '../../abstract/UploaderBlock'; | ||
| import { UploadSource } from '../../utils/UploadSource'; | ||
| import { EventType, InternalEventType } from '../UploadCtxProvider/EventEmitter'; | ||
| import { InternalEventType } from '../UploadCtxProvider/EventEmitter'; | ||
| import './url-source.css'; | ||
|
|
||
| type BaseInitState = InstanceType<typeof UploaderBlock>['init$']; |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/abstract/managers/TelemetryManager.ts (1)
90-108: Verify the behavioral impact of removing events from the exclusion list.
FILE_URL_CHANGEDandGROUP_CREATEDhave been removed from the excluded events list, which means they will now be sent to telemetry. Confirm this behavioral change is intentional and properly documented.#!/bin/bash # Description: Find all usages of FILE_URL_CHANGED and GROUP_CREATED event emissions to assess telemetry impact echo "=== Searching for FILE_URL_CHANGED emissions ===" rg -n --type=ts -C3 'EventType\.FILE_URL_CHANGED' -g '!node_modules/**' echo -e "\n=== Searching for GROUP_CREATED emissions ===" rg -n --type=ts -C3 'EventType\.GROUP_CREATED' -g '!node_modules/**' echo -e "\n=== Checking EventType definitions ===" rg -n --type=ts 'FILE_URL_CHANGED|GROUP_CREATED' -g '**/EventEmitter.ts'
🧹 Nitpick comments (1)
src/blocks/UrlSource/UrlSource.ts (1)
5-5: Remove unused EventType importCodeQL already complains that
EventTypeis never used. Please drop it to keep the bundle clean and satisfy the analyzer. Based on static analysis hints.-import { EventType, InternalEventType } from '../UploadCtxProvider/EventEmitter'; +import { InternalEventType } from '../UploadCtxProvider/EventEmitter';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/abstract/managers/TelemetryManager.ts(5 hunks)src/blocks/CameraSource/CameraSource.ts(3 hunks)src/blocks/FileItem/FileItem.ts(3 hunks)src/blocks/UploadCtxProvider/EventEmitter.ts(2 hunks)src/blocks/UploadList/UploadList.ts(3 hunks)src/blocks/UrlSource/UrlSource.ts(2 hunks)src/solutions/cloud-image-editor/CloudImageEditor.ts(1 hunks)src/solutions/file-uploader/inline/FileUploaderInline.ts(1 hunks)src/solutions/file-uploader/minimal/FileUploaderMinimal.ts(1 hunks)src/solutions/file-uploader/regular/FileUploaderRegular.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/blocks/CameraSource/CameraSource.ts
- src/blocks/UploadCtxProvider/EventEmitter.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/solutions/file-uploader/inline/FileUploaderInline.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
InternalEventType(8-12)
src/blocks/UploadList/UploadList.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
InternalEventType(8-12)
src/solutions/file-uploader/regular/FileUploaderRegular.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
InternalEventType(8-12)
src/solutions/file-uploader/minimal/FileUploaderMinimal.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
InternalEventType(8-12)
src/solutions/cloud-image-editor/CloudImageEditor.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
InternalEventType(8-12)
src/abstract/managers/TelemetryManager.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (3)
InternalEventKey(40-40)EventKey(38-38)InternalEventType(8-12)
src/blocks/FileItem/FileItem.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
InternalEventType(8-12)
src/blocks/UrlSource/UrlSource.ts (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
InternalEventType(8-12)
🪛 GitHub Check: CodeQL
src/blocks/FileItem/FileItem.ts
[notice] 16-16: Unused variable, import, function or class
Unused import EventType.
src/blocks/UrlSource/UrlSource.ts
[notice] 5-5: Unused variable, import, function or class
Unused import EventType.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
src/blocks/FileItem/FileItem.ts (1)
87-112: LGTM!The telemetry events for edit and remove actions are correctly implemented using
InternalEventType.ACTION_EVENT, providing valuable tracking for user interactions.src/blocks/UploadList/UploadList.ts (1)
41-72: LGTM!The telemetry events for add-more and clear-all actions are correctly implemented using
InternalEventType.ACTION_EVENT. The separation between internal telemetry events and public event emissions (UPLOAD_CLICK, DONE_CLICK) is appropriate.src/abstract/managers/TelemetryManager.ts (2)
5-6: LGTM!The introduction of
CommonEventTypeas a union ofInternalEventKey | EventKeyproperly unifies internal and external event handling while maintaining type safety.Also applies to: 11-11
36-38: LGTM!The migration from
EventTypeconstants toInternalEventTypeconstants for internal telemetry operations (INIT_SOLUTION, CHANGE_CONFIG, ACTION_EVENT) is consistent and correct throughout the file.Also applies to: 46-49, 67-67, 152-152
| import { throttle } from '../../utils/throttle'; | ||
| import { ExternalUploadSource } from '../../utils/UploadSource'; | ||
| import './file-item.css'; | ||
| import { EventType, InternalEventType } from '../UploadCtxProvider/EventEmitter'; |
There was a problem hiding this comment.
Remove unused import.
EventType is imported but never used in this file.
Apply this diff to remove the unused import:
-import { EventType, InternalEventType } from '../UploadCtxProvider/EventEmitter';
+import { InternalEventType } from '../UploadCtxProvider/EventEmitter';📝 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 { EventType, InternalEventType } from '../UploadCtxProvider/EventEmitter'; | |
| import { InternalEventType } from '../UploadCtxProvider/EventEmitter'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 16-16: Unused variable, import, function or class
Unused import EventType.
🤖 Prompt for AI Agents
In src/blocks/FileItem/FileItem.ts around line 16, the import statement pulls in
EventType which is not used; remove EventType from the import list (leave
InternalEventType) and update the import line accordingly so only used symbols
are imported.
Description
Checklist
Summary by CodeRabbit
Refactor
Chores