Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR integrates the Amplitude browser SDK into the extension’s analytics pipeline, refactors metric emission to use the SDK (with opt-out support), and adds a development-only analytics debug panel to inspect recent events/state.
Changes:
- Add
@amplitude/analytics-browserand migrateemitMetricto Amplitude SDK-based tracking with common context, opt-out syncing, and popup-close flushing. - Introduce an Analytics Debug section on the Debug page that displays SDK state plus a dev-only localStorage-backed recent-events buffer.
- Adjust webpack env wiring to centralize DefinePlugin values and inject
APP_VERSION/BUILD_TYPEfor analytics context.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Locks Amplitude SDK and transitive deps. |
| extension/package.json | Adds @amplitude/analytics-browser dependency. |
| extension/webpack.common.js | Centralizes DefinePlugin values; injects APP_VERSION and BUILD_TYPE. |
| extension/webpack.extension.js | Routes DEV_SERVER / DEV_EXTENSION into common config via merged env. |
| extension/webpack.dev.js | Passes .env AMPLITUDE_KEY into common config env and converts dev config to a factory. |
| extension/src/constants/env.ts | Adds typed APP_VERSION and BUILD_TYPE globals exported for analytics. |
| extension/src/constants/localStorageTypes.ts | Adds DEBUG_ANALYTICS_EVENTS localStorage key constant. |
| extension/src/helpers/metrics.ts | Replaces manual HTTP upload with Amplitude SDK; adds debug snapshots/subscriptions and recent-event buffer. |
| extension/src/popup/App.tsx | Initializes Amplitude at popup startup. |
| extension/src/popup/views/Debug.tsx | Adds Analytics Debug UI (SDK status + recent events) using useSyncExternalStore. |
| extension/src/popup/views/Debug/styles.scss | Styles for analytics debug status + events list. |
| extension/src/popup/views/Swap/index.tsx | Emits screen-view metrics once per step transition. |
| extension/src/popup/views/Send/index.tsx | Emits screen-view metrics once per step transition. |
| extension/src/popup/views/FullscreenSuccessMessage/index.tsx | Moves metric emission into a mount-only effect. |
| extension/src/popup/components/manageAssets/TrustlineError/index.tsx | Prevents repeated metric emission by adding empty deps array. |
| extension/src/popup/components/ErrorTracking/index.tsx | Broadens Sentry denyUrls to suppress Amplitude SDK network noise. |
| extension/src/popup/constants/metricsNames.ts | Adds swapAmountReview metric name. |
| extension/src/popup/locales/en/translation.json | Adds strings for analytics debug UI. |
| extension/src/popup/locales/pt/translation.json | Adds Portuguese strings for analytics debug UI. |
| extension/src/popup/views/tests/SignTransaction.test.tsx | Updates metrics helper mocks for new exports. |
| extension/src/popup/views/tests/Account.test.tsx | Updates metrics helper mocks for new exports. |
| config/jest/setupTests.tsx | Adds Amplitude SDK mock and expands metrics helper mock surface. |
| extension/README.md | Documents the dev-only Analytics Debug panel and behavior. |
| extension/.env.example | Adds example env file and notes for AMPLITUDE_KEY usage. |
| .github/workflows/submitBeta.yml | Attempts to pass AMPLITUDE_KEY/SENTRY_KEY at build time (currently broken YAML). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let recentEventsSnapshot: DebugEvent[] = []; | ||
| let debugListeners: Array<() => void> = []; | ||
|
|
There was a problem hiding this comment.
getRecentEvents() returns recentEventsSnapshot, but that snapshot is initialized as [] and never populated from localStorage on startup. As a result, the Analytics Debug panel won't show persisted events after a refresh/reopen until a new event is emitted (or a storage event fires from another tab), which contradicts the README behavior. Consider initializing recentEventsSnapshot from readPersistedEvents() at module load (dev-only), or calling invalidateSnapshot() when the first subscriber registers / on first getRecentEvents() call.
| * Returns first 8 + last 4 chars (e.g. "GABCDEFG…WXYZ"), which is enough | ||
| * for session correlation without storing the full address. | ||
| */ | ||
| const truncatePublicKey = (publicKey: string): string => |
There was a problem hiding this comment.
we actually already have a helper for this one in freighter/extension/src/helpers/stellar.ts called truncatedPublicKey
|
|
||
| // Initialize the snapshot from localStorage so the debug panel shows | ||
| // persisted events immediately after a page refresh (dev only). | ||
| let recentEventsSnapshot: DebugEvent[] = isDev ? readPersistedEvents() : []; |
There was a problem hiding this comment.
I see that we're adding a debug screen for dev environments that records the last 50 analytics calls. Can I ask what problem we're solving by implementing this? I don't necessarily think it's wrong! It just seems like we're adding a lot of code for a feature that we haven't discussed or prioritized as a team
There was a problem hiding this comment.
I added this more to keep in line with the mobile implementation, that we log the events locally as well for debugging, but honestly not sure how often we need to use it. for initial implementation it helped a lot, but it's a fair point / open if we need or want to keep it in the long run, as it likely wont change much after the implementation is done
@CassioMG wdyt? in regards to mobile as well. maybe instead we go the opposite way and remove it on both sides if it's not used too often (or at all)
There was a problem hiding this comment.
We've added it on mobile because by default we avoid sending any events to Amplitude when developing locally (DEV) to prevent flooding Amplitude with trash events, which means we don't use the Amplitude API KEY when running things locally on mobile. So this debug screen was added to help seeing the events locally without having to manually connect to Amplitude locally (which basically means changing 1-2 lines of code locally).
Tbh, I'm not strongly in favor of keeping this analytics debug screen as I also never use it. But one thing to notice is that on mobile the Analytics logs on the debug screen is just one of the sections of the debug screen. This same debug screen is used for setting other convenient debug values locally like overriding Blockaid responses or Transaction failure responses so it's easy to simulate those scenarios.
There was a problem hiding this comment.
Yeah, in dev mode in the extension, we just console.log metrics rather than sending them off to Amplitude. If you navigate around the app in dev, you can see the history of metrics that would have been emitted in your console. This conse is probably a little more accessible in the browser than in mobile. I'd suggest not adding this so we don't have to worry about maintaining a feature that doesn't seem to have much of a value add
There was a problem hiding this comment.
sounds good to me, let's remove it
There was a problem hiding this comment.
I think we can also remove it on mobile to be honest
There was a problem hiding this comment.
sounds good to me too. will clean it up 👌
There was a problem hiding this comment.
stellar/freighter-mobile#795 also created a ticket for mobile. if we have time during this sprint, I will pull it to my board during the week
| // We manage the user ID ourselves via `getUserId()` + localStorage so that | ||
| // we control the identifier format and lifecycle. Amplitude's default | ||
| // identity storage would create a second, SDK-managed identifier that could | ||
| // conflict with ours and is unnecessary given we call setUserId() manually. |
There was a problem hiding this comment.
hm, what is the benefit to us handling this ourselves rather than letting the SDK do it for us?
There was a problem hiding this comment.
good catch. it came from taking the mobile implementation as a base (there it's generated manually)
getUserId from Math.random -> Claude suggested a different approach -> I changed to this with crypto. but we can just leave it for the SDK directly
There was a problem hiding this comment.
@piyalbasu @leofelix077 I'm also not sure there is a lot of value in ourselves creating the id here but for the moment I'd keep it the same as mobile for consistency. We can remove it once we implement Freighter's user model so we'll have proper unique user ids which we could also use on Amplitude. How does that sound?
|
|
||
| // Firefox extensions (browser.* API) | ||
| const browserVersion = g?.browser?.runtime?.getManifest?.()?.version; | ||
| if (browserVersion) return browserVersion; |
There was a problem hiding this comment.
the browser lib should work for all browsers (including Chrome). You shouldn't need to distinguish between Chrome and Firefox
| if (browserVersion) return browserVersion; | ||
|
|
||
| // Dev server or unknown environment — use build-time constant | ||
| return APP_VERSION; |
There was a problem hiding this comment.
Why not try this first? There should be an APP_VERSION in almost all scenarios
There was a problem hiding this comment.
oh yes. it should always be available. we don't need this logic
| [STEPS.AMOUNT]: METRIC_NAMES.swapAmount, | ||
| [STEPS.CONFIRM_AMOUNT]: METRIC_NAMES.swapAmountReview, | ||
| [STEPS.SET_FROM_ASSET]: METRIC_NAMES.swapFrom, | ||
| }; |
| - run: > | ||
| yarn && yarn build:freighter-api && yarn build:extension:production | ||
| --env AMPLITUDE_KEY="${{ secrets.AMPLITUDE_KEY }}" SENTRY_KEY="${{ | ||
| secrets.SENTRY_KEY }}" |
There was a problem hiding this comment.
If we were going to add analytics/Sentry to Beta, I don't think we should use the Prod keys here.
IMO, I don't think we need Sentry tracking at all in Beta.
I can see an argument for being able to test Amplitude in Beta, but we should use a key specific to the beta environment for this so we don't pollute Production data
There was a problem hiding this comment.
the amplitude key itself can be the same, but we have different build types for prod/beta/dev, so it can be filtered by the different environments. so it shouldn't be a problem to segment and filter it properly without having polluted data
| /** | ||
| * Build type derived from webpack flags at compile time. | ||
| * - "development" — `yarn start` (dev server) | ||
| * - "beta" — `yarn build` (non-production extension build) |
There was a problem hiding this comment.
this actually isn't accurate. You'll see that submitBeta uses the yarn:build:production command. The reason for this is that we want the build to be as close as possible to the production build when we're QA'ing
| const lastEmittedStep = React.useRef<STEPS | null>(null); | ||
|
|
||
| // Emit a screen-view metric only once per step transition. | ||
| React.useEffect(() => { |
There was a problem hiding this comment.
| React.useEffect(() => { | |
| useEffect(() => { |
| const [activeStep, setActiveStep] = React.useState(STEPS.AMOUNT); | ||
| const lastEmittedStep = React.useRef<STEPS | null>(null); |
There was a problem hiding this comment.
| const [activeStep, setActiveStep] = React.useState(STEPS.AMOUNT); | |
| const lastEmittedStep = React.useRef<STEPS | null>(null); | |
| const [activeStep, setActiveStep] = useState(STEPS.AMOUNT); | |
| const lastEmittedStep = useRef<STEPS | null>(null); |
| const metricByStep: Partial<Record<STEPS, string>> = { | ||
| [STEPS.AMOUNT]: METRIC_NAMES.sendPaymentAmount, | ||
| [STEPS.PAYMENT_CONFIRM]: METRIC_NAMES.sendPaymentConfirm, | ||
| [STEPS.DESTINATION]: METRIC_NAMES.sendPaymentRecentAddress, | ||
| }; |
There was a problem hiding this comment.
this could probably live outside the useEffect since it's a static object
| @@ -31,6 +31,27 @@ export const Swap = () => { | |||
| const navigate = useNavigate(); | |||
| const location = useLocation(); | |||
| const [activeStep, setActiveStep] = React.useState(STEPS.AMOUNT); | |||
There was a problem hiding this comment.
| const [activeStep, setActiveStep] = React.useState(STEPS.AMOUNT); | |
| const [activeStep, setActiveStep] = useState(STEPS.AMOUNT); |
| # To test Amplitude locally, add the line below with your Amplitude key, then run: yarn start | ||
| # | ||
| # AMPLITUDE_KEY= | ||
| # |
There was a problem hiding this comment.
should we also add AMPLITUDE_EXPERIMENT_DEPLOYMENT_KEY here?
There was a problem hiding this comment.
I see it's being handled on this other PR: https://github.com/stellar/freighter/pull/2666/changes#diff-5e8b4d2df7c32bf287ba46a22ace2c8dff10bd859a615b97173d2da3c8e4d758R8
| const context: Record<string, unknown> = { | ||
| publicKey: activePublicKey ? truncatePublicKey(activePublicKey) : "N/A", | ||
| platform: METRICS_PLATFORM, | ||
| platformVersion: navigator.userAgent, |
There was a problem hiding this comment.
I'm not sure we should log the whole userAgent here, maybe we could try coarsening to browser name + major version
| publicKey: activePublicKey ? truncatePublicKey(activePublicKey) : "N/A", | ||
| platform: METRICS_PLATFORM, | ||
| platformVersion: navigator.userAgent, | ||
| network: (networkDetails?.networkName ?? "").toUpperCase(), |
There was a problem hiding this comment.
on mobile we are sending the network identifier here, e.g. "PUBLIC" | "TESTNET"
I think we should do the same on extension for consistency, instead of using the network name which will log it as "MAIN NET" | "TEST NET"
| }; | ||
|
|
||
| const context: Record<string, unknown> = { | ||
| publicKey: activePublicKey ? truncatePublicKey(activePublicKey) : "N/A", |
There was a problem hiding this comment.
on mobile we are sending the whole public key without any truncation
There was a problem hiding this comment.
this came from a comment from opencode to obfuscate it, not to have a direct identifier to any wallet. wdyt? it's not like we can easily track a wallet owner, but there are some extra info available that could track down an approximate wallet + location combination. wdyt?
There was a problem hiding this comment.
@leofelix077 humm yeah in this case let's also truncate on mobile so we keep both in sync. Would you mind doing that?
There was a problem hiding this comment.
for sure. I added to the same other ticket stellar/freighter-mobile#795 (as more of a catch all, not to have 2 tiny tickets) 1. to remove the debug local logging and 2. truncate the addresses
| network: (networkDetails?.networkName ?? "").toUpperCase(), | ||
| connectionType: nav.connection?.type ?? "unknown", | ||
| appVersion: getAppVersion(), | ||
| bundleId: BUILD_TYPE, |
There was a problem hiding this comment.
I think using the build type as the bundleId is too generic considering both extension and mobile send events to the same project.
I'd suggest making it something like bundleId: extension.${BUILD_TYPE} , so we will have "org.stellar.freighterdev" / "org.stellar.freighterwallet" for mobile and "extension.dev" / "extension.beta" / "extension.production" for extension which removes any ambiguity.
Wdyt?
There was a problem hiding this comment.
oh yes. makes sense. also makes easier for readability and finding it at a glance
CassioMG
left a comment
There was a problem hiding this comment.
LGTM - I'm in favor of removing the Debug analytics screen, also please double check everything looks good on Amplitude now that the id is generated locally - thanks
|
@CassioMG
|



#2538
Screen.Recording.2026-03-23.at.12.07.09.mov
Screen.Recording.2026-03-23.at.12.08.00.mov