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.

Sign up for GitHub

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

Mixpanel experimentation (wip) #1252

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
+281 −30
Draft

Mixpanel experimentation (wip) #1252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions web/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,20 +48,23 @@
"@parcel/watcher": "~2.2.0",
"@types/amqplib": "^0.10.1",
"@types/busboy": "^1.5.0",
"@types/mixpanel-browser": "^2.47.3",
"@types/react": "^18.2.14",
"@types/react-dom": "^18.2.7",
"@types/styled-components": "^5.1.26",
"@typescript-eslint/eslint-plugin": "^5.58.0",
"@typescript-eslint/parser": "^5.61.0",
"@typescript-eslint/utils": "^5.58.0",
"@wagmi/cli": "^1.3.0",
"crypto-browserify": "^3.12.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider moving crypto-browserify to dependencies

The addition of crypto-browserify suggests that the project needs cryptographic functionality in a browser environment. However, it's unusual to see this as a devDependency. If this package is used at runtime (which is typically the case), it should be moved to the dependencies section instead.

Consider applying this change:

-  "devDependencies": {
-    ...
-    "crypto-browserify": "^3.12.0",
-    ...
-  },
+  "dependencies": {
+    ...
+    "crypto-browserify": "^3.12.0",
+    ...
+  },

Committable suggestion was skipped due to low confidence.

"eslint": "^8.38.0",
"eslint-config-prettier": "^8.8.0",
"eslint-import-resolver-parcel": "^1.10.6",
"eslint-plugin-react": "^7.33.0",
"eslint-plugin-react-hooks": "^4.6.0",
"lru-cache": "^7.18.3",
"parcel": "2.8.3",
"string_decoder": "^1.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider moving string_decoder to dependencies

The addition of string_decoder suggests that the project needs string decoding functionality in a browser environment. However, like crypto-browserify, it's unusual to see this as a devDependency. If this package is used at runtime (which is typically the case), it should be moved to the dependencies section instead.

Consider applying this change:

-  "devDependencies": {
-    ...
-    "string_decoder": "^1.3.0",
-    ...
-  },
+  "dependencies": {
+    ...
+    "string_decoder": "^1.3.0",
+    ...
+  },

Committable suggestion was skipped due to low confidence.

"typescript": "^4.9.5"
},
"dependencies": {
Expand All @@ -81,6 +84,7 @@
"ethers": "^5.7.2",
"graphql": "^16.7.1",
"graphql-request": "~6.1.0",
"mixpanel-browser": "^2.47.0",
"moment": "^2.29.4",
"overlayscrollbars": "^2.3.0",
"overlayscrollbars-react": "^0.5.2",
Expand Down
2 changes: 2 additions & 0 deletions web/src/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ import Cases from "./pages/Cases";
import Dashboard from "./pages/Dashboard";
import Courts from "./pages/Courts";
import DisputeTemplateView from "./pages/DisputeTemplateView";
import mixpanel from "./utils/mixpanel";

const App: React.FC = () => {
mixpanel.track("App");
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider optimizing the Mixpanel tracking call.

While adding analytics is beneficial, the current implementation may lead to excessive tracking. The "App" event will be logged on every render of the App component, which could result in redundant data and potentially impact performance.

Consider the following improvements:

  1. Use a more descriptive event name, e.g., "App Loaded" or "App Rendered".
  2. Implement a mechanism to track only on initial load or at a reasonable interval. For example:
import { useEffect } from 'react';

const App: React.FC = () => {
  useEffect(() => {
    mixpanel.track("App Loaded");
  }, []); // Empty dependency array ensures this runs only once on mount

  // ... rest of the component
};
  1. If you need to track re-renders, consider adding more context to the event, such as the reason for the re-render or the current app state.

return (
<StyledComponentsProvider>
<QueryClientProvider>
Expand Down
54 changes: 33 additions & 21 deletions web/src/components/CasesDisplay/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import styled from "styled-components";
import Search from "./Search";
import StatsAndFilters from "./StatsAndFilters";
import CasesGrid, { ICasesGrid } from "./CasesGrid";
import useTracking from "../../hooks/useTracking";

const StyledHR = styled.hr`
margin-top: 24px;
Expand All @@ -22,26 +23,37 @@ const CasesDisplay: React.FC<ICasesDisplay> = ({
casesPerPage,
title = "Cases",
className,
}) => (
<div {...{ className }}>
<h1>{title}</h1>
<Search />
<StatsAndFilters />
<StyledHR />
{disputes.length > 0 ? (
<CasesGrid
{...{
disputes,
currentPage,
setCurrentPage,
numberDisputes,
casesPerPage,
}}
/>
) : (
<h1>wow no cases</h1>
)}
</div>
);
}) => {
useTracking("CasesDisplay", {
disputes,
currentPage,
setCurrentPage,
numberDisputes,
casesPerPage,
title,
className,
});
Comment on lines +27 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refining the data passed to useTracking.

While the useTracking hook is correctly implemented, consider refining the data passed to it. Currently, all component props are being tracked, including function props like setCurrentPage. It's recommended to only track relevant data to avoid unnecessary overhead and potential issues with serializing function props.

For example, you might want to track only the following:

useTracking("CasesDisplay", {
  currentPage,
  numberDisputes,
  casesPerPage,
  title,
});

This would provide meaningful analytics without including potentially sensitive or unnecessary data.

return (
<div {...{ className }}>
<h1>{title}</h1>
<Search />
<StatsAndFilters />
<StyledHR />
{disputes.length > 0 ? (
<CasesGrid
{...{
disputes,
currentPage,
setCurrentPage,
numberDisputes,
casesPerPage,
}}
/>
) : (
<h1>wow no cases</h1>
)}
</div>
);
};

export default CasesDisplay;
2 changes: 2 additions & 0 deletions web/src/components/ConnectWallet/AccountDisplay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { landscapeStyle } from "styles/landscapeStyle";
import { useAccount, useNetwork, useEnsAvatar, useEnsName } from "wagmi";
import Identicon from "react-identicons";
import { shortenAddress } from "utils/shortenAddress";
import { useIdentify } from "../../hooks/useTracking";

const Container = styled.div`
display: flex;
Expand Down Expand Up @@ -119,6 +120,7 @@ export const AddressOrName: React.FC = () => {
address,
chainId: 1,
});
useIdentify(address);
return <label>{data ?? (address && shortenAddress(address))}</label>;
};

Expand Down
3 changes: 3 additions & 0 deletions web/src/components/ConnectWallet/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import { useAccount, useNetwork, useSwitchNetwork } from "wagmi";
import { useWeb3Modal } from "@web3modal/react";
import { Button } from "@kleros/ui-components-library";
import { SUPPORTED_CHAINS, DEFAULT_CHAIN } from "consts/chains";
import useTracking from "../../hooks/useTracking";
import AccountDisplay from "./AccountDisplay";

export const SwitchChainButton: React.FC = () => {
const { switchNetwork, isLoading } = useSwitchNetwork();
useTracking("Switch Network");
const handleSwitch = () => {
if (!switchNetwork) {
console.error("Cannot switch network. Please do it manually.");
Expand Down Expand Up @@ -36,6 +38,7 @@ const ConnectButton: React.FC = () => {
const ConnectWallet: React.FC = () => {
const { chain } = useNetwork();
const { isConnected } = useAccount();
useTracking("Connect Wallet", { chain });
if (isConnected) {
if (chain && chain.id !== DEFAULT_CHAIN) {
return <SwitchChainButton />;
Expand Down
23 changes: 23 additions & 0 deletions web/src/hooks/useTracking.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// useTracking.ts
import { useEffect } from "react";
import mixpanel from "../utils/mixpanel";
import crypto from "crypto";

const useTracking = (eventName: string, props?: object) => {
useEffect(() => {
mixpanel.track(eventName, {
pathname: window.location.pathname,
...(props ?? {}),
});
}, [eventName, props]);
};

export const useIdentify = (userId: string | undefined, props?: object) => {
useEffect(() => {
if (userId) {
mixpanel.identify(crypto.createHash("sha256").update(userId).digest("hex"));
}
}, [userId, props]);
};
Comment on lines +15 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling, remove unused parameter, and check crypto availability

The useIdentify hook has a good foundation, but there are several areas for improvement:

  1. Error handling: Add try-catch block to handle potential errors in the hashing process.
  2. Unused parameter: The props parameter is currently unused. Either use it or remove it.
  3. Crypto availability: Check for the availability of the crypto module before using it.

Here's a suggested implementation addressing these points:

export const useIdentify = (userId: string | undefined) => {
  useEffect(() => {
    if (userId && typeof crypto !== 'undefined') {
      try {
        const hashedUserId = crypto.createHash("sha256").update(userId).digest("hex");
        mixpanel.identify(hashedUserId);
      } catch (error) {
        console.error("Error identifying user:", error);
      }
    }
  }, [userId]);
};

This implementation:

  1. Adds a try-catch block for error handling.
  2. Removes the unused props parameter.
  3. Checks for the availability of the crypto module before using it.

Note: If you plan to use the props parameter in the future, keep it but add it to the dependency array of useEffect.


export default useTracking;
6 changes: 6 additions & 0 deletions web/src/pages/Cases/CaseDetails/Evidence/SubmitEvidenceModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { uploadFormDataToIPFS } from "utils/uploadFormDataToIPFS";
import { useWalletClient, usePublicClient } from "wagmi";
import { EnsureChain } from "components/EnsureChain";
import { prepareWriteDisputeKitClassic } from "hooks/contracts/generated";
import mixpanel from "../../../../utils/mixpanel";

const StyledModal = styled(Modal)`
position: absolute;
Expand Down Expand Up @@ -82,6 +83,11 @@ const SubmitEvidenceModal: React.FC<{
});
await wrapWithToast(async () => await walletClient.writeContract(request), publicClient).then(
() => {
mixpanel.track("submitEvidence", {
pathname: window.location.pathname,
cid,
evidenceGroup,
});
setMessage("");
close();
}
Expand Down
11 changes: 11 additions & 0 deletions web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { useCourtDetails } from "hooks/queries/useCourtDetails";
import { wrapWithToast } from "utils/wrapWithToast";
import { isUndefined } from "utils/index";
import { EnsureChain } from "components/EnsureChain";
import mixpanel from "../../../../utils/mixpanel";
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider relocating the Mixpanel utility for better organization.

The current import statement uses a relative path that goes up four directory levels. This might indicate that the Mixpanel utility could be placed in a more accessible location within the project structure. Consider moving it to a common utilities folder that's easier to access from various parts of the application.


export enum ActionType {
allowance = "allowance",
Expand Down Expand Up @@ -87,6 +88,10 @@ const StakeWithdrawButton: React.FC<IActionButton> = ({
wrapWithToast(async () => await increaseAllowance().then((response) => response.hash), publicClient).finally(
() => {
setIsSending(false);
mixpanel.track("increaseAllowance", {
pathname: window.location.pathname,
amount: increaseAllowanceConfig.request.args[1].toString(),
});
}
);
}
Expand All @@ -104,6 +109,12 @@ const StakeWithdrawButton: React.FC<IActionButton> = ({
.then(() => setIsPopupOpen(true))
.finally(() => {
setIsSending(false);
mixpanel.track("setStake", {
pathname: window.location.pathname,
action: isStaking ? "stake" : "withdraw",
courtId: id,
stakeChange: (isStaking ? "" : "-") + parsedAmount.toString(),
});
});
}
};
Expand Down
9 changes: 9 additions & 0 deletions web/src/utils/mixpanel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import mixpanel from "mixpanel-browser";

mixpanel.init(process.env.REACT_APP_MIXPANEL_TOKEN!, {
debug: true,
track_pageview: true,
persistence: "localStorage",
});

export default mixpanel;
Loading