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

Remove error analytics #448

Merged
merged 5 commits into from
May 19, 2024
Merged

Remove error analytics #448

merged 5 commits into from
May 19, 2024

Conversation

nop33
Copy link
Member

@nop33 nop33 commented Mar 12, 2024

I want to avoid the issue of floading the analytics with errors because of a useEffect, like it happened with the GH API rate limits. I went through both our wallets in search of the places we report things to PostHog and removed some. @mvaivre I would like to review them with you and think of a way to ensure that we won't end up with the same problem in the future.

@nop33 nop33 requested a review from mvaivre March 12, 2024 10:40
Copy link

changeset-bot bot commented Mar 12, 2024

⚠️ No Changeset found

Latest commit: a41c987

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mvaivre
Copy link
Member

mvaivre commented Mar 20, 2024

Should we plan a little call to go through this?

@nop33
Copy link
Member Author

nop33 commented Mar 20, 2024

Yes

@nop33 nop33 changed the base branch from fix-wc-sign to next May 13, 2024 13:09
@nop33
Copy link
Member Author

nop33 commented May 13, 2024

@mvaivre as we are preparing to fix the analytics issue, I would like to merge this so I am requesting your review. As the description of the PR says, I went through all places where we call the Posthog API to see where there could be a potential issue in the future. I removed a few API requests in places that I deemed necessary.

I also posted a question in Posthog support here: https://posthog.com/questions/how-to-limit-the-amount-of-requests-sent-from-my-app-to-posthog

This issue is related: #453

Let me know how you'd like to proceed.

@nop33
Copy link
Member Author

nop33 commented May 14, 2024

@mvaivre when you have the time, please check my last comment here 🙏

@nop33
Copy link
Member Author

nop33 commented May 15, 2024

kind ping @mvaivre for my previous comments 🙏

@nop33 nop33 added 📱 MW Mobile wallet 🖥 DW Desktop wallet labels May 16, 2024
@@ -64,6 +64,7 @@ const WalletSwitchButton = ({ style }: WalletSwitchButtonProps) => {
setNbOfTaps(0)
}

// TODO: Shouldn't this be 69?
Copy link
Member

Choose a reason for hiding this comment

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

😨 you're right. I wanted to start counting from -1 to make it less obvious, but apparently I considered this as non vital and made a mistake 🙃

@nop33 nop33 merged commit 768ddcf into next May 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🖥 DW Desktop wallet 📱 MW Mobile wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants