feat: add Brave browser detection #125
Conversation
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds Brave browser detection capability to the analytics SDK by introducing an asynchronous check for the Brave browser's native API (navigator.brave.isBrave()). The core implementation resides in EventFactory's new isBraveBrowser() method, which checks for the Brave-specific navigator property and calls its async detection method. This detection is performed during context generation for every event, adding Brave identification to the analytics context object. Since Brave's detection API is inherently asynchronous, this required converting the entire event creation pipeline from synchronous to asynchronous - propagating through EventFactory.create() → EventManager.addEvent() → FormoAnalytics tracking methods. TypeScript type definitions were added to the global Navigator interface to support the Brave API. The change includes code formatting standardization (double quotes) and proper error handling for the detection logic. This fits into the SDK's broader goal of enriching analytics events with browser context information, which is particularly valuable given Brave's unique privacy features and tracking prevention mechanisms.
Changed Files
| Filename | Score | Overview |
|---|---|---|
| src/global.d.ts | 5/5 | Added TypeScript type definitions for Brave browser detection by extending Navigator interface with optional brave property |
| src/lib/event/type.ts | 4/5 | Changed IEventManager and IEventFactory interface signatures from synchronous to asynchronous (breaking change) |
| src/lib/event/EventFactory.ts | 4/5 | Added async isBraveBrowser() method and converted entire class from sync to async to support detection on every event |
| src/lib/event/EventManager.ts | 4/5 | Converted addEvent method to async to support awaiting EventFactory's now-async create() method |
| src/FormoAnalytics.ts | 4/5 | Updated to await eventManager.addEvent() calls and applied formatting standardization throughout the file |
Confidence score: 4/5
- This PR requires careful review but appears safe to merge with moderate risk due to breaking API changes
- Score reflects the breaking change to public interfaces (sync to async), potential performance impact from detection on every event, and the need to verify all call sites properly handle the new async nature; no critical bugs detected but the architectural impact is significant
- Pay close attention to src/lib/event/type.ts (interface breaking changes), src/lib/event/EventFactory.ts (detection implementation called on every event), and verify all consumers of these APIs throughout the codebase properly await the now-async methods
Sequence Diagram
sequenceDiagram
participant User
participant FormoAnalytics
participant EventFactory
participant Navigator
participant EventQueue
User->>FormoAnalytics: "Initialize SDK or Trigger Event"
FormoAnalytics->>EventFactory: "generateContext()"
EventFactory->>Navigator: "navigator.brave?.isBrave()"
alt Brave Browser
Navigator-->>EventFactory: "true"
else Not Brave
Navigator-->>EventFactory: "false"
end
EventFactory->>EventFactory: "Set is_brave_browser in context"
EventFactory-->>FormoAnalytics: "Return enriched context with is_brave_browser"
FormoAnalytics->>EventFactory: "create(event, address, userId)"
EventFactory->>EventFactory: "Generate event with context including is_brave_browser"
EventFactory-->>FormoAnalytics: "Return IFormoEvent"
FormoAnalytics->>EventQueue: "enqueue(formoEvent)"
EventQueue-->>User: "Event tracked with Brave detection"
5 files reviewed, 1 comment
55837d3 to
e1619d5
Compare
|
bugbot run |
| } | ||
|
|
||
| try { | ||
| this.cachedIsBrave = |
There was a problem hiding this comment.
Does this have to be async?
I thought window.navigator.brave is sync
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds Brave browser detection capability to the analytics SDK by introducing asynchronous browser identification. The changes convert the entire event generation pipeline from synchronous to asynchronous to accommodate Brave's promise-based navigator.brave.isBrave() API. TypeScript type definitions are added in global.d.ts to properly type the Brave API, while EventFactory implements the detection logic with a caching mechanism to avoid repeated API calls. The asynchronous nature propagates through IEventFactory and IEventManager interfaces, requiring EventManager.addEvent() and all factory methods to return Promises. Finally, FormoAnalytics.trackEvent() now properly awaits event addition. The implementation adds an is_brave_browser boolean field to all event contexts, enabling the analytics platform to track Brave browser usage alongside other browser metrics.
Changed Files
| Filename | Score | Overview |
|---|---|---|
| src/global.d.ts | 5/5 | Adds TypeScript type definitions for Brave's browser detection API to the Navigator interface |
| src/lib/event/type.ts | 4/5 | Updates interfaces to make event creation and management methods asynchronous |
| src/lib/event/EventFactory.ts | 4/5 | Implements Brave browser detection with caching and converts all event generation methods to async |
| src/lib/event/EventManager.ts | 4/5 | Converts addEvent method to async and awaits eventFactory.create() |
| src/FormoAnalytics.ts | 4/5 | Adds await to eventManager.addEvent() call plus extensive formatting improvements |
Confidence score: 4/5
- This PR requires careful review as it introduces a breaking API change by converting synchronous methods to asynchronous throughout the event pipeline.
- Score reflects that while the implementation is sound with proper error handling and caching, the architectural shift from sync to async affects the entire event generation flow. Points deducted for: (1) Breaking change in public interfaces requiring all consumers to update their code to await these methods, (2) Potential issues if existing code calling
addEvent()doesn't properly handle the returned Promise, (3) The async nature adds overhead to every event generation call even with caching. - Pay close attention to
src/lib/event/type.ts(interface changes),src/lib/event/EventFactory.ts(core detection logic), andsrc/lib/event/EventManager.ts(async propagation). Verify that all callers of these methods throughout the codebase have been updated to handle Promises correctly.
Sequence Diagram
sequenceDiagram
participant User
participant FormoAnalytics
participant EventFactory
participant Navigator
participant EventManager
participant EventQueue
User->>FormoAnalytics: trackEvent(type, payload)
FormoAnalytics->>EventManager: addEvent(event, address, userId)
EventManager->>EventFactory: create(event, address, userId)
EventFactory->>EventFactory: generateContext(context)
EventFactory->>Navigator: navigator.brave?.isBrave()
alt Brave browser detected
Navigator-->>EventFactory: true
else Not Brave or unavailable
Navigator-->>EventFactory: false/undefined
end
EventFactory->>EventFactory: Cache result in cachedIsBrave
EventFactory->>EventFactory: Merge context with is_brave_browser flag
EventFactory-->>EventManager: return IFormoEvent
EventManager->>EventQueue: enqueue(formoEvent, callback)
EventQueue-->>User: callback(err, _, data)
5 files reviewed, 1 comment
| this.cachedIsBrave = | ||
| typeof globalThis.navigator?.brave !== "undefined" && | ||
| (await globalThis.navigator?.brave?.isBrave()); |
There was a problem hiding this comment.
style: The check for navigator.brave presence doesn't guarantee the API is available. Consider wrapping the entire API call in try-catch since the API could be removed or blocked by extensions
| this.cachedIsBrave = false; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Race Condition in Brave Browser Detection
There's a race condition in isBraveBrowser() where concurrent calls can bypass the cachedIsBrave check before the initial asynchronous detection completes. This leads to redundant calls to navigator.brave.isBrave() and defeats the intended caching mechanism.
| page_url: globalThis.location.href, | ||
| library_name: "Formo Web SDK", | ||
| library_version, | ||
| is_brave_browser: isBraveBrowser, |
There was a problem hiding this comment.
make this more generic:
browser: 'brave'
in case we need to support detection for other browsers
| class EventFactory implements IEventFactory { | ||
| private cachedIsBrave: boolean | null = null; | ||
|
|
||
| private async isBraveBrowser(): Promise<boolean> { |
There was a problem hiding this comment.
This logic doesn't belong in the event factory, extract it out to a browsers.ts module
Note
Adds Brave browser detection to event context and converts event generation/queueing to async across EventFactory/EventManager with corresponding type updates.
EventFactorymethods async and enrich context withis_brave_browservia cachednavigator.brave.isBrave().EventManager.addEventto async and awaiteventFactory.create; update interfaces insrc/lib/event/type.tsto returnPromise.FormoAnalytics, awaiteventManager.addEventintrackEvent.navigator.braveinsrc/global.d.ts.FormoAnalytics.Written by Cursor Bugbot for commit e1619d5. This will update automatically on new commits. Configure here.