Repository navigation
fix(analytics): detect browser for Chromium-based browsers - #2648
creeperkatze wants to merge 7 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 | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check ✅ Passed The title clearly summarizes the main change: detecting Chromium-based browsers in analytics. Description check ✅ Passed The description includes an overview and concrete manual testing steps. It omits the Related Issue section, but the remaining content is complete and relevant. 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.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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 |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
I've been trying to reproduce this I think button in It looks interesting, but i need to test it manually before any approval. And resolve conflict with Thanks |
|
Thanks! I pushed the branch
Before, |
|
Also resolved the |
| chrome: 'chrome', | ||
| edge: 'edge', | ||
| firefox: 'firefox', | ||
| chromium: 'other_chromium', | ||
| brave: 'other_chromium', | ||
| opera: 'other_chromium', | ||
| vivaldi: 'other_chromium', | ||
| yandex: 'other_chromium', | ||
| naver: 'other_chromium', | ||
| }; |
There was a problem hiding this comment.
@creeperkatze Thanks a lot, now i see difference.
But there's unknown for safari, because it isn't listed there.
Another problem is all browsers below chromium is other_chromium, IMO this should be named 'opera': 'opera' and etc.
But i've been wondering if it's possible and good solution to use Bowser.BROWSER_MAP there, instead of our BROWSER_MAP, what do you think?
There was a problem hiding this comment.
Glad you could reproduce it!
I kept Moderok's values in line with their own SDK, which only sends chrome, edge, firefox, other_chromium and unknown (Safari and others end up as unknown). Not sure if their API accepts more than that, if it does, happy to pass the browser through directly.
There was a problem hiding this comment.
As for Bowser.BROWSER_MAP, it just maps Bowser's IDs to display names (e.g. edge becomes "Microsoft Edge"). Moderok still needs its own values.
There was a problem hiding this comment.
@creeperkatze Can you provide me a link for that method in Moderok's API docs?
I want to check it, because it looks like, it's ready to go.
There was a problem hiding this comment.
They don't have public API docs unfortunately, but their SDK is open source:
There was a problem hiding this comment.
@creeperkatze But we can't skip safari, let's find a way to add it, maybe implement it in upstream?
There was a problem hiding this comment.
Agreed, but that's up to Moderok, their API only knows those five values and Safari isn't something they support yet. If we send safari now, their API might reject the events instead of counting them as unknown.
There was a problem hiding this comment.
Even if I opened a PR at moderok-sdk adding Safari, their backend is closed source, so it'd still need Moderok to accept it on their end.
Overview
The
browseruser property was always empty for Chromium-based browsers when built, sinceua-parser-jscouldn't read the user agent in MV3 service workers. It also couldn't tell browsers like Brave apart from Chrome.This PR replaces it with bowser, which also reads client hints.
browsernow reports IDs likechrome,edge,braveandfirefox.Also:
optimizeDeps.includeentry forua-parser-js.ua-parser-jsfrom the excluded packages inupgrade-deps.ts.other_chromiuminstead ofunknown.Manual Testing
bun run dev:buildinpackages/analyticsand load.output/chrome-mv3in Chrome and Edge.trackevent should showuser.properties.browseraschrome/edge. Before this change it wasundefined.