-
Notifications
You must be signed in to change notification settings - Fork 408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Track most searched safe apps #1827
Conversation
Branch preview✅ Deploy successful! https://apps_search_analytics--webcore.review-web-core.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
@@ -29,6 +30,14 @@ const useSafeAppsFilters = (safeAppsList: SafeAppData[]): ReturnType => { | |||
|
|||
const filteredApps = useAppsFilterByOptimizedForBatch(filteredAppsByQueryAndCategories, optimizedWithBatchFilter) | |||
|
|||
const debouncedSearchQuery = useDebounce(query, 500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought: what about tracking this on unmount instead, e.g. when the chosen app is open? This is highly dependent on the speed of the typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might miss some data points then i.e. users who search for an app but don't open it. Also I think we already track whenever a safe app is opened via the SAFE_APPS_EVENTS.OPEN_APP
event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 2 seconds then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or better, store the list of distinct queries, and track on unmount.
If a query starts with the same symbols as an existing query, overwrite it instead of adding to the set.
If we end up with stuff like gov
, govern
, safe gover
all tracked in the same session, that would be too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bigger debounce should be OK too, though. Maybe 5 seconds.
The search is also pretty slow and blocks typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might miss some data points then i.e. users who search for an app but don't open it.
The only time that would happen is if the user closes the browser, otherwise unmount always occurs when the user navigates within the app. It's certainly something to consider though.
Also I think we already track whenever a safe app is opened via the
SAFE_APPS_EVENTS.OPEN_APP
event.
I'm thinking about over-tracking of the search term. Perhaps onBlur
would be a better reference point as the useEffect
may run >1 times for slow typers. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that there already was a debounce of 400ms in place so I removed my change and adjusted the other hook instead. So currently, the safe apps list is updated after 400ms of not typing and thats where we also dispatch the analytics event. We could increase that number but imo it would make the UX worse because it would take longer to see the results of the search but also the two events should be linked.
Was this call removed because of over-tracking? If not I suggest we restore the previous state and create a separate ticket for future improvements on the mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow how the instant search debounce is related to the tracking debounce. We can afford to re-render the app list on every keystroke, but sending all the intermediate query states to GA is no bueno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can imagine that if the list updates too fast it could "interrupt" the user. I assume that it could also lead to users not typing out longer search terms anymore because they already see a result. Not sure if this is good or bad but at least from an analytics point of view I would argue it would be harder to understand how the search is being used. There is some interesting info on this topic which I think we should explore before making such a change.
As for the analytics, lets add a second debounce (2s) as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some inefficiency in the re-rendering too, it shouldn't be so slow. In Safari, the search bar is super sluggish.
f64db25
to
899a232
Compare
Looks good, like the snapshot in the description Question, Is the idea to track the most searched apps? or should we track anything the user writes? I think they are technically different things. Or maybe I'm just over thinking. Anyways it looks fine and I'm okay shipping it how it is right now |
I think the idea is to track anything the user searches for. On the analytics end we can then write queries to find out things like most searched terms. |
What it solves
Resolves #1814
How this PR fixes it
How to test it
Screenshots
Checklist