feat: add missing events related to charts #416
Conversation
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude finished @vinu-deriv's task —— View job Code Review Complete ✅I've reviewed the latest changes to this PR. Great work addressing the previous feedback! The duplicate line issue has been fixed, comprehensive test coverage has been added, and the typo has been corrected. Here's my detailed analysis: Summary of Changes Since Last ReviewFixes Applied:
Latest Commits:
Current Status: Near Perfect! 🎯The implementation is now in excellent shape. However, there's still one remaining issue from the previous review that should be addressed: 🟡 Medium Priority IssueMissing Validation for Drawing Tools OperationsLocation: Issue: The
Impact: Could send incomplete analytics events with empty Current Code: const getDrawingToolsFormAnalyticsData = (state: keyof typeof STATE_TYPES, option: TStateChangeOption = {}) => {
const { drawing_tool_name = '', pxthickness = '', color_name = '', is_open } = option;
const drawing_tools_event_type = 'ce_drawing_tools_form_v2';
const payload = {
event_type: drawing_tools_event_type,
} as TPayload;
switch (state) {
case STATE_TYPES.DRAWING_TOOLS_MODAL_OPEN:
payload.data = {
action: ACTION.OPEN,
};
break;
case STATE_TYPES.DRAWING_TOOLS_ADD: // ⚠️ No validation here
payload.data = {
action: ACTION.ADD_DRAWING_TOOL,
drawing_tool_name, // Could be empty!
// ...Recommended Fix: const getDrawingToolsFormAnalyticsData = (state: keyof typeof STATE_TYPES, option: TStateChangeOption = {}) => {
const { drawing_tool_name = '', pxthickness = '', color_name = '', is_open } = option;
const drawing_tools_event_type = 'ce_drawing_tools_form_v2';
const payload = {
event_type: drawing_tools_event_type,
} as TPayload;
// Add validation for operations that require drawing_tool_name
if (
(state === STATE_TYPES.DRAWING_TOOLS_ADD ||
state === STATE_TYPES.DRAWING_TOOLS_DELETE ||
state === STATE_TYPES.DRAWING_TOOLS_EDIT_PX ||
state === STATE_TYPES.DRAWING_TOOLS_EDIT_COLOR) &&
!drawing_tool_name
) {
return {};
}
switch (state) {
// ... rest of the switch statement
}
};Note: This is defensive programming to maintain consistency with the rest of the codebase and prevent potential analytics data quality issues. ✅ What's Excellent1. Previous Issues ResolvedAll critical and high-priority issues from the last review have been addressed:
2. Test Coverage is Now Complete
The test coverage is thorough and follows the existing patterns perfectly. 3. Code Quality
4. Event ArchitectureThe analytics implementation is well-structured:
🔍 Minor Observations (Not Blockers)1. Type Safety for
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @vinu-deriv's task —— View job Code Review Complete ✅I've thoroughly reviewed this PR that adds analytics tracking for charts, drawing tools, and crosshair interactions. The implementation shows significant improvement from previous reviews, with most issues resolved. Here's my comprehensive analysis: SummaryThis PR successfully adds comprehensive analytics tracking for:
Overall Assessment: The code is well-structured with good test coverage. However, there's one critical issue that needs attention before merging. 🔴 Critical IssueMissing Validation for Drawing Tools OperationsLocation: Severity: High - Data Quality Issue Problem: The Pattern Comparison:
Impact:
Current Code: const getDrawingToolsFormAnalyticsData = (state: keyof typeof STATE_TYPES, option: TStateChangeOption = {}) => {
const { drawing_tool_name = '', pxthickness = '', color_name = '' } = option;
const drawing_tools_event_type = 'ce_drawing_tools_form_v2';
const payload = {
event_type: drawing_tools_event_type,
} as TPayload;
switch (state) {
case STATE_TYPES.DRAWING_TOOLS_MODAL_OPEN:
payload.data = {
action: ACTION.OPEN,
};
break;
case STATE_TYPES.DRAWING_TOOLS_ADD: // ⚠️ No validation!
payload.data = {
action: ACTION.ADD_DRAWING_TOOL,
drawing_tool_name, // Could be empty string!
pxthickness,
color_name,
};
break;
// ... similar issues for DELETE, EDIT_PX, EDIT_COLOR
}
return payload;
};Recommended Fix: const getDrawingToolsFormAnalyticsData = (state: keyof typeof STATE_TYPES, option: TStateChangeOption = {}) => {
const { drawing_tool_name = '', pxthickness = '', color_name = '' } = option;
const drawing_tools_event_type = 'ce_drawing_tools_form_v2';
const payload = {
event_type: drawing_tools_event_type,
} as TPayload;
// Add validation for operations that require drawing_tool_name
if (
(state === STATE_TYPES.DRAWING_TOOLS_ADD ||
state === STATE_TYPES.DRAWING_TOOLS_DELETE ||
state === STATE_TYPES.DRAWING_TOOLS_EDIT_PX ||
state === STATE_TYPES.DRAWING_TOOLS_EDIT_COLOR) &&
!drawing_tool_name
) {
return {};
}
switch (state) {
// ... rest of the implementation
}
};Testing Recommendation: it('should return empty object when drawing_tool_name is missing for ADD operation', () => {
expect(getChartAnalyticsData(STATE_TYPES.DRAWING_TOOLS_ADD, {})).toEqual({});
});
it('should return empty object when drawing_tool_name is missing for DELETE operation', () => {
expect(getChartAnalyticsData(STATE_TYPES.DRAWING_TOOLS_DELETE, {})).toEqual({});
});✅ What's Excellent1. Previous Issues Fully ResolvedAll issues from previous reviews have been addressed:
2. Outstanding Test CoverageLocation: The test suite is thorough and well-organized:
Test Quality: Tests follow existing patterns, use descriptive names, and validate both success and failure cases. 3. Clean Code Architecture
4. Latest Commit QualityThe latest commit ( 🟢 Code Quality ObservationsStrengths
Minor Observations (Not Blockers)1. Type Consistency for
|
vinu-deriv
merged commit 7a09864
into
deriv-com:refactor/champion-chart
4 participants
Description
(Brief description of what this PR does)
Related Issue: Fixes #(issue)
Type of Change
Testing
Screenshots
(Add screenshots if UI changes)
Before:
After: