Repository navigation
fix(analytics): keep frontend metadata separate from optional arguments - #2649
creeperkatze wants to merge 5 commits into
Conversation
✅ Deploy Preview for creative-fairy-df92c4 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and concisely describes the main change: keeping frontend metadata separate from optional analytics arguments. |
| Description check | ✅ Passed | The description includes an overview and concrete manual testing steps with expected results. The template’s Related Issue section is omitted, but it is conditional and does not prevent the descriptio… |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
@coderabbitai review |
✅ Action performedReview finished.
|
| port.onMessage.addListener((message: AnalyticsMessage) => { | ||
| const { meta } = message; | ||
| // Pass every argument explicitly, so meta doesn't shift into an omitted optional one | ||
| switch (message.fn) { | ||
| case 'identify': | ||
| return void analytics.identify( | ||
| message.args[0], | ||
| message.args[1], | ||
| meta, | ||
| ); | ||
| case 'page': | ||
| return void analytics.page(message.args[0], meta); | ||
| case 'track': | ||
| return void analytics.track(message.args[0], message.args[1], meta); | ||
| case 'captureException': | ||
| return void analytics.captureException( | ||
| message.args[0], | ||
| message.args[1], | ||
| meta, | ||
| ); | ||
| case 'setEnabled': | ||
| return void analytics.setEnabled(message.args[0]); | ||
| } |
There was a problem hiding this comment.
| port.onMessage.addListener((message: AnalyticsMessage) => { | |
| const { meta } = message; | |
| // Pass every argument explicitly, so meta doesn't shift into an omitted optional one | |
| switch (message.fn) { | |
| case 'identify': | |
| return void analytics.identify( | |
| message.args[0], | |
| message.args[1], | |
| meta, | |
| ); | |
| case 'page': | |
| return void analytics.page(message.args[0], meta); | |
| case 'track': | |
| return void analytics.track(message.args[0], message.args[1], meta); | |
| case 'captureException': | |
| return void analytics.captureException( | |
| message.args[0], | |
| message.args[1], | |
| meta, | |
| ); | |
| case 'setEnabled': | |
| return void analytics.setEnabled(message.args[0]); | |
| } | |
| port.onMessage.addListener((message: AnalyticsMessage) => { | |
| const { meta, fn } = message; | |
| // Pass every argument explicitly, so meta doesn't shift into an omitted optional one | |
| switch (fn) { | |
| case 'identify': | |
| return analytics.identify( | |
| message.args[0], | |
| message.args[1], | |
| meta, | |
| ); | |
| case 'page': | |
| return analytics.page(message.args[0], meta); | |
| case 'track': | |
| return analytics.track(message.args[0], message.args[1], meta); | |
| case 'captureException': | |
| return analytics.captureException( | |
| message.args[0], | |
| message.args[1], | |
| meta, | |
| ); | |
| case 'setEnabled': | |
| return analytics.setEnabled(message.args[0]); | |
| } |
There was a problem hiding this comment.
Thanks, applied your suggestion and fixed the conflict!
Button 1 goes through autoTrack, which always passes properties, so the bug doesn't show there. It only happens when you leave out an optional argument, like the analytics.track('some-event') example from the README.
Try adding void analytics.track('some-event') to the demo popup. Before this PR, the event's properties end up being the popup's metadata and meta.url is background.js. After, it's correct.
Not a breaking change since the public API stays the same. Only the internal message between the popup and the background changes.
…end-metadata # Conflicts: # packages/analytics/modules/analytics/client.ts


Overview
Calls from the popup, options pages or content scripts that left out an optional argument, like
track('some-event')oridentify('some-user-id')from the README, received the page metadata in that argument's place. The metadata is now sent as its own field.Manual Testing
packages/analytics/entrypoints/popup/main.ts), addvoid analytics.track('no-props');.bun run dev:buildinpackages/analyticsand load.output/chrome-mv3in Chrome.trackevent should have no properties, andmetashould have the popup's URL and a session ID. Before this change, the metadata showed up inevent.propertiesandmeta.urlwasbackground.js.