feat: side-by-side edit mode hover analytics #34185
Conversation
alex-golovanov
requested review from
a team and
marks0351
and removed request for
a team
WalkthroughThis update introduces analytics functionality for side-by-side widgets in a client application. New action creators and reducers handle recording and resetting analytics. A bespoke saga and custom hook identify side-by-side editor mode and trigger analytics on canvas interactions. Constants and utility files streamline widget-specific hover data capture. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Canvas as LayoutSystemBasedCanvas
participant AnalyticsSaga
participant AnalyticsAction
User->>Canvas: Hover on Canvas
Canvas->>AnalyticsAction: sendAnalyticsForSideBySideHover()
AnalyticsAction->>AnalyticsSaga: trigger(data)
AnalyticsSaga->>AnalyticsAction: recordAnalyticsForSideBySideWidgetHover()
note over AnalyticsSaga: Records hover data
AnalyticsSaga->>AnalyticsSaga: Apply selection, hover state logic
AnalyticsSaga->>AnalyticsAction: resetAnalyticsForSideBySideHover()
AnalyticsAction->>AnalyticsSaga: Success Response
User->>Canvas: Leaves hover
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
IDE Product
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
app/client/src/actions/analyticsActions.ts (1)
11-28: The action creators for side-by-side widget analytics are correctly implemented. Consider adding comments to each action creator to explain their purpose and usage within the application.app/client/src/sagas/AnalyticsSaga.ts (2)
14-25: Consider using consistent import grouping. Group all redux-saga related imports together for better readability.
Line range hint
39-54: Wrap the declarations inside the switch case in blocks to restrict their scope and prevent access by other switch clauses.case ReduxActionTypes.UPDATE_ACTION_INIT: + { const { action, pageName } = payload as { action: Action; pageName: string; }; const cleanActionConfiguration = cleanValuesInObjectForHashing( action.actionConfiguration, ); const actionConfigurationHash: string = yield generateHashFromString( JSON.stringify(cleanActionConfiguration), ); const originalActionId = get( action, `${RequestPayloadAnalyticsPath}.originalActionId`, action.id, ); AnalyticsUtil.logEvent("SAVE_ACTION", { actionName: action.name, pageName: pageName, originalActionId: originalActionId, actionId: action.id, hash: actionConfigurationHash, actionType: action.pluginType, actionPlugin: action.pluginId, }); + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- app/client/src/actions/analyticsActions.ts (1 hunks)
- app/client/src/ce/constants/ReduxActionConstants.tsx (1 hunks)
- app/client/src/ce/sagas/index.tsx (2 hunks)
- app/client/src/ce/utils/analyticsUtilTypes.ts (1 hunks)
- app/client/src/layoutSystems/CanvasFactory.tsx (3 hunks)
- app/client/src/layoutSystems/common/widgetName/WidgetNameLayer.tsx (1 hunks)
- app/client/src/layoutSystems/common/widgetName/index.tsx (9 hunks)
- app/client/src/layoutSystems/constants.ts (1 hunks)
- app/client/src/layoutSystems/styles.module.css (1 hunks)
- app/client/src/reducers/uiReducers/analyticsReducer.ts (1 hunks)
- app/client/src/sagas/AnalyticsSaga.ts (2 hunks)
- app/client/src/selectors/analyticsSelectors.tsx (1 hunks)
- app/client/src/utils/hooks/useIsInSideBySideEditor.ts (1 hunks)
Files skipped from review due to trivial changes (4)
- app/client/src/layoutSystems/common/widgetName/WidgetNameLayer.tsx
- app/client/src/layoutSystems/constants.ts
- app/client/src/layoutSystems/styles.module.css
- app/client/src/selectors/analyticsSelectors.tsx
Additional context used
Biome
app/client/src/sagas/AnalyticsSaga.ts
[error] 39-42: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 43-45: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 46-48: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 50-54: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
app/client/src/layoutSystems/common/widgetName/index.tsx
[error] 136-141: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (7)
app/client/src/utils/hooks/useIsInSideBySideEditor.ts (1)
11-19: The implementation ofuseIsInSideBySideEditorcorrectly determines the side-by-side editor mode based on the IDE view mode and the current pathname. Well done on maintaining clear and concise logic.app/client/src/sagas/AnalyticsSaga.ts (2)
71-88: The new sagasendSideBySideWidgetHoverAnalyticsEventSagaeffectively captures the hover state and logs the event. Ensure that thewidgetTypesarray is always defined to avoid potential runtime errors.
90-112: TherouteChangeInSideBySideModeSagafunction handles navigation changes effectively. However, ensure that theinvokedByandpathnameproperties are always present in the payload to avoid potential errors.app/client/src/ce/sagas/index.tsx (1)
Line range hint
54-114: The integration ofsendSideBySideWidgetHoverAnalyticsEventSagainto the sagas array is correctly implemented. This ensures that the new analytics functionality will be active and monitored.app/client/src/layoutSystems/common/widgetName/index.tsx (1)
195-200: The use ofuseEffectto dispatch analytics based on thecurrentActivitystate is a smart integration. However, ensure thatwidgetTypeis always defined to avoid sending incomplete analytics data.Verification successful
The search results indicate that
widgetTypeis used extensively throughout the codebase, including in the fileapp/client/src/layoutSystems/common/widgetName/index.tsx. However, the results do not provide direct evidence thatwidgetTypeis always defined when dispatching analytics.To ensure
widgetTypeis always defined, we need to verify its initialization and usage within the specific context of theuseEffecthook inindex.tsx.
The search results show multiple usages of
widgetTypeinindex.tsx, but they do not reveal its definition or initialization. To ensurewidgetTypeis always defined, we need to locate its declaration and initialization within the file.
The search results show that
widgetTypeis destructured from thepropsobject in theWidgetNameComponentfunction. This indicates thatwidgetTypeis passed as a prop to the component, which suggests it should always be defined when the component is used correctly.
widgetTypeis destructured frompropsin theWidgetNameComponentfunction.- It is used in various places within the component, including the
useEffecthook for dispatching analytics.Given this context, it appears that
widgetTypeis reliably defined as long as theWidgetNameComponentis used correctly with the appropriate props.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `widgetType` is always defined when dispatching analytics. # Test: Search for the definition and usage of `widgetType`. Expect: Always defined. rg --type python $'widgetType'Length of output: 60
Script:
#!/bin/bash # Description: Verify that `widgetType` is always defined when dispatching analytics. # Test: Search for the definition and usage of `widgetType`. Expect: Always defined. rg --type js,ts 'widgetType'Length of output: 58
Script:
#!/bin/bash # Description: Verify that `widgetType` is always defined when dispatching analytics. # Test: Search for the definition and usage of `widgetType`. Expect: Always defined. rg --type ts 'widgetType'Length of output: 56247
Script:
#!/bin/bash # Description: Verify that `widgetType` is always defined when dispatching analytics in `index.tsx`. # Test: Search for the definition and initialization of `widgetType` in `index.tsx`. rg 'widgetType' app/client/src/layoutSystems/common/widgetName/index.tsxLength of output: 498
Script:
#!/bin/bash # Description: Verify the definition and initialization of `widgetType` in `index.tsx`. # Test: Extract the lines around the usage of `widgetType` to find its declaration and initialization. rg -C 5 'widgetType' app/client/src/layoutSystems/common/widgetName/index.tsxLength of output: 2693
app/client/src/ce/utils/analyticsUtilTypes.ts (1)
357-358: The addition ofCANVAS_HOVERto theEventNametype aligns with the PR's objectives to enhance analytics for side-by-side canvas interactions. This is a crucial update for tracking new analytics events.app/client/src/ce/constants/ReduxActionConstants.tsx (1)
932-939: The newly added Redux action constants for side-by-side widget hover analytics are well-named and align with the PR objectives. Ensure that these constants are used consistently across sagas, reducers, and action creators.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/src/layoutSystems/common/widgetName/index.tsx (1)
Line range hint
1-232: Check for any potential simplifications inWidgetNameComponent, especially around conditions and hooks usage.- selectWidget && - selectWidget( - SelectionRequestType.One, - [widgetId], - NavigationMethod.CanvasClick, - ); + selectWidget?.( + SelectionRequestType.One, + [widgetId], + NavigationMethod.CanvasClick, + );Tools
Biome
[error] 136-141: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- app/client/src/actions/analyticsActions.ts (1 hunks)
- app/client/src/ce/constants/ReduxActionConstants.tsx (1 hunks)
- app/client/src/ce/sagas/index.tsx (2 hunks)
- app/client/src/ce/utils/analyticsUtilTypes.ts (1 hunks)
- app/client/src/layoutSystems/CanvasFactory.tsx (3 hunks)
- app/client/src/layoutSystems/common/widgetName/WidgetNameLayer.tsx (1 hunks)
- app/client/src/layoutSystems/common/widgetName/index.tsx (9 hunks)
- app/client/src/layoutSystems/constants.ts (1 hunks)
- app/client/src/layoutSystems/styles.module.css (1 hunks)
- app/client/src/reducers/uiReducers/analyticsReducer.ts (1 hunks)
- app/client/src/sagas/AnalyticsSaga.ts (2 hunks)
- app/client/src/selectors/analyticsSelectors.tsx (1 hunks)
- app/client/src/utils/hooks/useIsInSideBySideEditor.ts (1 hunks)
Files skipped from review due to trivial changes (4)
- app/client/src/layoutSystems/common/widgetName/WidgetNameLayer.tsx
- app/client/src/layoutSystems/constants.ts
- app/client/src/layoutSystems/styles.module.css
- app/client/src/selectors/analyticsSelectors.tsx
Additional context used
Biome
app/client/src/sagas/AnalyticsSaga.ts
[error] 39-42: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 43-45: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 46-48: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 50-54: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
app/client/src/layoutSystems/common/widgetName/index.tsx
[error] 136-141: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (15)
app/client/src/utils/hooks/useIsInSideBySideEditor.ts (1)
11-19: The hookuseIsInSideBySideEditorcorrectly determines the side-by-side editor mode based on the IDE view mode and the pathname. Ensure that theEditorViewMode.SplitScreenis the correct constant for side-by-side mode as per your application's constants.app/client/src/actions/analyticsActions.ts (4)
11-16: The action creatorrecordAnalyticsForSideBySideWidgetHovercorrectly dispatches an action with the widget type as payload. Ensure that theReduxActionTypes.RECORD_ANALYTICS_FOR_SIDE_BY_SIDE_WIDGET_HOVERis correctly defined in your constants.
18-20: The action creatorsendAnalyticsForSideBySideHoveris well-defined. Verify that the corresponding saga handles this action type appropriately.
22-24: The action creatorrecordAnalyticsForSideBySideNavigationis correctly implemented. Ensure that this action is triggered in the appropriate user interactions within the application.
26-28: The action creatorresetAnalyticsForSideBySideHoveris set up to reset the analytics state. This is crucial for maintaining accurate analytics data.app/client/src/reducers/uiReducers/analyticsReducer.ts (3)
27-34: The handler forRESET_ANALYTICS_FOR_SIDE_BY_SIDE_HOVERcorrectly resets the hover analytics state to its initial state. This is essential for accurate state management in analytics.
35-43: The handler forRECORD_ANALYTICS_FOR_SIDE_BY_SIDE_NAVIGATIONcorrectly sets thenavigatedstate. Ensure this handler is triggered in the correct context of user navigation.
44-55: The handler forRECORD_ANALYTICS_FOR_SIDE_BY_SIDE_WIDGET_HOVERappends the widget type to thewidgetTypesarray. This implementation supports the accumulation of multiple widget types, which aligns with the PR objectives.app/client/src/layoutSystems/CanvasFactory.tsx (1)
40-49: ThehandleMouseLeaveevent handler correctly dispatches thesendAnalyticsForSideBySideHoveraction when the mouse leaves the canvas and enters another element within the layout wrapper. This implementation is crucial for capturing user interactions in the side-by-side editor mode.app/client/src/sagas/AnalyticsSaga.ts (3)
90-112: Ensure the logic inrouteChangeInSideBySideModeSagacorrectly handles all specified navigation and view modes.
71-88: Wrap declarations in a block within the switch statement to restrict scope and prevent potential bugs.case ReduxActionTypes.UPDATE_ACTION_INIT: { + { const { action, pageName } = payload as { action: Action; pageName: string; }; const cleanActionConfiguration = cleanValuesInObjectForHashing( action.actionConfiguration, ); const actionConfigurationHash: string = yield generateHashFromString( JSON.stringify(cleanActionConfiguration), ); const originalActionId = get( action, `${RequestPayloadAnalyticsPath}.originalActionId`, action.id, ); AnalyticsUtil.logEvent("SAVE_ACTION", { actionName: action.name, pageName: pageName, originalActionId: originalActionId, actionId: action.id, hash: actionConfigurationHash, actionType: action.pluginType, actionPlugin: action.pluginId, }); + } }Likely invalid or redundant comment.
14-30: Ensure new imports are used appropriately and check for any unused imports.Verification successful
All new imports in the file
app/client/src/sagas/AnalyticsSaga.tsare used appropriately.
getIdeCanvasSideBySideHoverStateEditorViewModerecordAnalyticsForSideBySideNavigationresetAnalyticsForSideBySideHoverrouteChangedNavigationMethodgetIDEViewModeJS_COLLECTION_EDITOR_PATHWIDGETS_EDITOR_BASE_PATHScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all new imports are used in the file. # Test: Search for unused imports. Expect: No output if all imports are used. rg --files-with-matches --type python 'import { sendSideBySideWidgetHoverAnalyticsEventSaga }' app/client/src/sagas/AnalyticsSaga.tsLength of output: 280
Script:
#!/bin/bash # Description: Verify that all new imports are used in the file. # List of imports to verify imports=( "getIdeCanvasSideBySideHoverState" "EditorViewMode" "recordAnalyticsForSideBySideNavigation" "resetAnalyticsForSideBySideHover" "routeChanged" "NavigationMethod" "getIDEViewMode" "JS_COLLECTION_EDITOR_PATH" "WIDGETS_EDITOR_BASE_PATH" ) # Check usage of each import for import in "${imports[@]}"; do echo "Checking usage of import: $import" rg "$import" app/client/src/sagas/AnalyticsSaga.ts || echo "Import not used: $import" doneLength of output: 3116
app/client/src/ce/sagas/index.tsx (1)
Line range hint
54-114: Confirm thatsendSideBySideWidgetHoverAnalyticsEventSagais correctly imported and registered in the sagas array.app/client/src/ce/utils/analyticsUtilTypes.ts (1)
357-358: Confirm the addition of theCANVAS_HOVERevent type and ensure it is used appropriately in analytics logging.app/client/src/ce/constants/ReduxActionConstants.tsx (1)
932-939: The new Redux action constants for side-by-side hover analytics are well-named and follow the existing naming conventions. Ensure that these constants are used consistently across the application.
app/client/src/layoutSystems/common/widgetName/WidgetNameLayer.tsx
Outdated
Show resolved
Hide resolved
hetunandu
added
ok-to-test
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9582426402. |
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/src/sagas/AnalyticsSaga.ts (1)
Line range hint
45-60: Local variable declarations within theUPDATE_ACTION_INITcase of thesendAnalyticsEventSagafunction are not block-scoped, which might lead to unexpected behavior if other cases are added to this switch statement.case ReduxActionTypes.UPDATE_ACTION_INIT: { + const { action, pageName } = payload as { + action: Action; + pageName: string; + }; + const cleanActionConfiguration = cleanValuesInObjectForHashing( + action.actionConfiguration, + ); + const actionConfigurationHash: string = yield generateHashFromString( + JSON.stringify(cleanActionInterface), + ); + + const originalActionId = get( + action, + `${RequestPayloadAnalyticsPath}.originalActionId`, + action.id, + ); + + AnalyticsUtil.logEvent("SAVE_ACTION", { + actionName: action.name, + pageName: pageName, + originalActionId: originalActionId, + actionId: action.id, + hash: actionConfigurationHash, + actionType: action.pluginType, + actionPlugin: action.pluginId, + }); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/layoutSystems/common/AnalyticsWrapper/AnalyticsWrapper.tsx (1 hunks)
- app/client/src/sagas/AnalyticsSaga.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/layoutSystems/common/AnalyticsWrapper/AnalyticsWrapper.tsx
Additional context used
Biome
app/client/src/sagas/AnalyticsSaga.ts
[error] 45-48: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 49-51: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 52-54: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 56-60: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
Additional comments not posted (5)
app/client/src/sagas/AnalyticsSaga.ts (5)
14-36: The imports and constants setup looks clean and well-organized.
77-94: ThesendSideBySideWidgetHoverAnalyticsEventSagafunction correctly implements the logic to track widget hover analytics in side-by-side editor mode. The use ofSetforwidgetTypesto ensure uniqueness is appropriate.
96-118: TherouteChangeInSideBySideModeSagafunction effectively handles route changes. It checks if the navigation method was a canvas click in the split-screen mode and logs appropriate analytics. This aligns well with the PR's objectives.
[APROVED]
121-145: ThefocusWidgetInSideBySideModeSagafunction correctly identifies the widget and records its type for analytics purposes when the editor is in side-by-side mode. This is crucial for achieving the detailed analytics tracking required by the PR.
148-158: The root saga setup usingallandtakeEveryis correctly configured to handle the various analytics-related events. This setup ensures that the sagas are properly registered and will respond to the corresponding actions.
|
Deploy-Preview-URL: https://ce-34185.dp.appsmith.com |
alex-golovanov
added
ok-to-test
Description
The purpose of this PR is to collect data about hovered canvas and widgets and send it to analytics platform.
Fixes #33159
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9584384829
Commit: 9cec247
Cypress dashboard.
Tags: ``
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
AnalyticsWrappercomponent for handling analytics events within layout systems.useIsInSideBySideEditorto check for side-by-side editor mode.Bug Fixes
Refactor