Brand Agent Clarity 1st Party Integration#1040
Brand Agent Clarity 1st Party Integration#1040moemenahmed-n wants to merge 10 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a first-party Brand Agent integration to Clarity for tracking client-side events from the Brand Agent chat system. The integration creates a new event emitter API on the window object that allows the Brand Agent to report user interactions like bubble shows, nudges, and chat messages.
Changes:
- Adds a new
BrandAgentevent type (53) and corresponding TypeScript definitions - Creates a new
brand-agentmodule with event handling and encoding logic - Exposes a
window.BrandAgentClarityAPI with on/off/emit methods for event communication
Reviewed changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/clarity-js/types/global.d.ts | Adds TypeScript interface for window.BrandAgentClarity API |
| packages/clarity-js/types/data.d.ts | Adds BrandAgent event type (53) to the Event enum |
| packages/clarity-js/types/brand-agent.d.ts | Defines BrandAgentClarityEventName enum and BrandAgentClarityEvent interface |
| packages/clarity-js/src/clarity.ts | Integrates BrandAgent module into the main modules array |
| packages/clarity-js/src/brand-agent/index.ts | Implements event emitter and handler registration for Brand Agent events |
| packages/clarity-js/src/brand-agent/encode.ts | Encodes Brand Agent events into Clarity's token format for transmission |
| packages/clarity-js/src/brand-agent/blank.ts | Provides empty implementation for conditional compilation (not currently used) |
| BubbleShown, | ||
| NudgeClicked, | ||
| AgentMessageSent, | ||
| UserMessageSent, |
There was a problem hiding this comment.
why do we need something new? Cant we reuse Action enum from agent.d.ts?
| Focus = 50, | ||
| CustomElement = 51, | ||
| Chat = 52, | ||
| BrandAgent = 53, |
There was a problem hiding this comment.
why not reuse Chat? why have a new event?
| @@ -0,0 +1,4 @@ | |||
| import * as brandAgent from "@src/dynamic/brandagent/brandagent"; | |||
There was a problem hiding this comment.
why is this a standalone folder? this can live within agent folder
| @@ -0,0 +1,21 @@ | |||
| import { Event, Token } from "@clarity-types/data"; | |||
There was a problem hiding this comment.
reuse encode in agent folder
3944d93 to
139f2aa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 11 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
| import { Data } from "clarity-js"; | ||
| import { Constant, DataEvent } from "../types/data"; | ||
| import { BrandAgentEventName } from "clarity-js/types/data"; |
There was a problem hiding this comment.
This import is only used for a type assertion, but it is written as a value import from clarity-js/types/data. Depending on TS compiler settings/bundling, this can emit a runtime import for a .d.ts-only path and fail at runtime. Prefer a type-only reference (e.g., import type ...) or reference it via the existing Data namespace (Data.BrandAgentEventName) so no runtime dependency is introduced.
| export function start(): void { | ||
| if (!window.BrandAgentClarity) { | ||
| window.BrandAgentClarity = { | ||
| on(name: string, cb: (e: any) => void) { | ||
| if (!handlers.has(name)) { handlers.set(name, new Set()); } | ||
| handlers.get(name)!.add(cb); | ||
| }, | ||
| off(name: string, cb: (e: any) => void) { | ||
| handlers.get(name)?.delete(cb); | ||
| }, | ||
| emit(name: string, e: any) { | ||
| handlers.get(name)?.forEach((fn: (e: any) => void) => fn(e)); | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| window.BrandAgentClarity.on("agentEvent", handleAgentEvent); | ||
|
|
||
| // Register stop callback with main Clarity | ||
| if (typeof window !== "undefined" && (window as any).clarity) { | ||
| (window as any).clarity("register", stop); | ||
| } | ||
| } | ||
|
|
||
| export function stop(): void { | ||
| if (window.BrandAgentClarity) { | ||
| window.BrandAgentClarity.off("agentEvent", handleAgentEvent); | ||
| } | ||
| handlers.clear(); | ||
| } |
There was a problem hiding this comment.
window.BrandAgentClarity is only initialized the first time this bundle runs, but its on/off/emit functions close over the module-local handlers map. On subsequent loads of this dynamic bundle (Clarity does stop/start on SPA navigations), window.BrandAgentClarity already exists so the new module instance’s handlers is unused, yet stop() clears the new map instead of the map actually used by BrandAgentClarity. This can cause handler state to persist across restarts and makes cleanup inconsistent. Consider storing the handler map on window.BrandAgentClarity itself (or otherwise ensuring stop() clears the same handler store that on/off/emit uses) so reloads remain correct.
| case Data.Event.BrandAgent: | ||
| if (payload.brandAgent === undefined) { payload.brandAgent = []; } | ||
| payload.brandAgent.push(data.decode(entry) as BrandAgentEvent); | ||
| break; |
There was a problem hiding this comment.
New BrandAgent event decoding logic was added, but there’s no corresponding test coverage validating that a payload containing Event.BrandAgent ends up in DecodedPayload.brandAgent with the expected name/msg/cid fields. Adding a small test case would help prevent regressions in the decoder and keep event compatibility stable.
|
@moemenahmed-n please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Brand Agent Clarity 1st party integration for client-side events.