Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add analytics to install modal #1189

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

chakra-guy
Copy link
Collaborator

@chakra-guy chakra-guy commented Jan 9, 2025

Explanation

We added analytics for modal interactions—viewed, button clicked, and toggle changed—but omitted QR scan detection as it's currently not feasible. We also fixed a minor bug. These changes ensure we capture essential user actions within the install modal.

Here are some log from the socket server:

sdk_modal_viewed

{"event":{"event":"sdk_modal_viewed","properties":{"dappId":"DevNext","event":"sdk_modal_viewed","platform":"web-desktop","sdkVersion":"0.31.4","source":"direct","tab":"desktop","title":"DevNext","url":"http://localhost:3333","userId":"5a374dcd2e5eb762b527af3a5bab6072a4d24493"},"userId":"5a374dcd2e5eb762b527af3a5bab6072a4d24493"},"level":"info","message":"Segment batch"}

sdk_modal_button_clicked

{"event":{"event":"sdk_modal_button_clicked","properties":{"button_type":"install_extension","dappId":"DevNext","event":"sdk_modal_button_clicked","platform":"web-desktop","sdkVersion":"0.31.4","source":"direct","tab":"desktop","title":"DevNext","url":"http://localhost:3333","userId":"5a374dcd2e5eb762b527af3a5bab6072a4d24493"},"userId":"5a374dcd2e5eb762b527af3a5bab6072a4d24493"},"level":"info","message":"Segment batch"}

sdk_modal_toggle_changed

{"event":{"event":"sdk_modal_toggle_changed","properties":{"dappId":"DevNext","event":"sdk_modal_toggle_changed","platform":"web-desktop","sdkVersion":"0.31.4","source":"direct","title":"DevNext","toggle":"mobile_to_desktop","url":"http://localhost:3333","userId":"5a374dcd2e5eb762b527af3a5bab6072a4d24493"},"userId":"5a374dcd2e5eb762b527af3a5bab6072a4d24493"},"level":"info","message":"Segment batch"}

References

No direct issue references.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@chakra-guy chakra-guy changed the title chore: adding core analytics structure chore: add analytics to install modal Jan 9, 2025
Copy link

socket-security bot commented Jan 9, 2025

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@chakra-guy chakra-guy marked this pull request as ready for review January 10, 2025 09:41
@chakra-guy chakra-guy requested a review from a team as a code owner January 10, 2025 09:41
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 74.28%. Comparing base (4151089) to head (dd21986).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../RemoteConnection/ModalManager/showInstallModal.ts 0.00% 5 Missing ⚠️
packages/sdk/src/ui/InstallModal/Modal-web.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1189      +/-   ##
==========================================
- Coverage   74.40%   74.28%   -0.13%     
==========================================
  Files         181      181              
  Lines        4306     4313       +7     
  Branches     1057     1061       +4     
==========================================
  Hits         3204     3204              
- Misses       1102     1109       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

packages/sdk-install-modal-web/stencil.config.ts Outdated Show resolved Hide resolved
packages/sdk/package.json Outdated Show resolved Hide resolved
@chakra-guy chakra-guy requested a review from abretonc7s January 10, 2025 12:17
Copy link

Copy link
Collaborator

@ecp4224 ecp4224 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one nit

@@ -47,6 +51,14 @@ export class InstallModal {
this.setTab(this.preferDesktop ? 1 : 2);

this.i18nInstance = new SimpleI18n();

this.trackAnalytics.emit({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit: it's probably best to have this be invoked inside componentDidLoad() lifecycle method. This may be more correct to do, but you'll need to add the lifecycle method to these components

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants