feat(analytics): track settings changed event(s)#718
Conversation
Deployment results
Logs #26470815019 |
kjmitchelljr
requested review from
DarianM and
sidvishnoi
and removed request for
DarianM
| current: object, | ||
| ): ChangedFields { | ||
| const result: Record<string, boolean | string> = {} | ||
| walk(prev as PlainObject | undefined, current as PlainObject, '', result) |
There was a problem hiding this comment.
Consider using this package: https://www.npmjs.com/package/microdiff
| changed: ChangedFields, | ||
| ): void { | ||
| if (Object.keys(changed).length === 0) return | ||
| trackFn(`${tool}_settings_changed`, { changed }) |
There was a problem hiding this comment.
Do we want this changed. prefix in all properties given event name says _changed?
| } | ||
| } | ||
|
|
||
| export function trackSettingsEvent( |
There was a problem hiding this comment.
Given this is used only in useSaveProfile, consider inlining it there instead of adding to general analytics.
| | [event: `${Tool}_profile_saved`] | ||
| | [event: `${Tool}_script_generated`] | ||
| | [event: `${Tool}_settings_changed`, data: SettingsChangedData] |
There was a problem hiding this comment.
Should we include this data in saved/generated events instead of a new event? Given either of these events will be fired immediately after this change event.
|
|
||
| snapshots.set(activeTab, current) | ||
| store.profilesUpdate.delete(activeTab) | ||
| persistSnapshots() |
There was a problem hiding this comment.
Why persist all snapshots when committing only active profile?
There was a problem hiding this comment.
@sidvishnoi I can verify why this is the case, initially I just took the logic that was in commitProfile() in the separate stores and brought it over. Since the localStorage.setItem() was being called twice once in commitProfile() and once commitAllProfiles() I decided to wrap in a function, but could leave as it is only a one-liner
There was a problem hiding this comment.
Something to address later on..
| @@ -0,0 +1,47 @@ | |||
| export type ChangedFields = Partial<Record<`field.${string}`, boolean | string>> | |||
There was a problem hiding this comment.
This will take up less space (visually as well as in storage):
| export type ChangedFields = Partial<Record<`field.${string}`, boolean | string>> | |
| export type ChangedFields = Partial<Record<`f.${string}`, boolean | string>> |
2 participants
Summary
Implements detailed event tracking for settings changes across Banner, Offerwall, and Widget tools
Changes
createToolStoreUtilsto integrate diffing directly into the profile commit${tool}_settings_changedevents only when changes occurRelated Issues
Closes #693