Repository navigation
Add analytics of user behaviour - #96
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds analytics helpers (trackDownload, trackLogScaleToggle, debounced trackPlotControls), records download events in DownloadSelect and LocationDownload, updates privacy wording, and extends unit and e2e tests and helpers to exercise the new tracking. ChangesDownload Analytics Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Do accounting of download events under a single event named 'download', but also send information on (1) the type of download, corresponding to whether the download came via the location-based download UI or the other (file-based), (2) if applicable, which locations were selected, and (3) which files were downloaded. This is sent to posthog, and we can configure the way we want to slice that data there; this can and will be changed, so the analytics sent from vaxviz should not make assumptions about that.
e88ed5b to
7a8ff6c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
==========================================
+ Coverage 98.39% 98.43% +0.04%
==========================================
Files 37 38 +1
Lines 870 896 +26
Branches 247 256 +9
==========================================
+ Hits 856 882 +26
Misses 9 9
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/utils/analytics.spec.ts (1)
93-104: ⚡ Quick winAdd edge-case coverage for
trackDownload.Current tests only validate the success path. Please add cases for “analytics disabled” and “capture throws” so privacy and resilience behavior stay protected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/utils/analytics.spec.ts` around lines 93 - 104, Add two unit tests for trackDownload: one where analytics is disabled (e.g., set the flag or env/config that trackDownload checks) and assert posthog.capture is NOT called; and one where posthog.capture is stubbed to throw and assert that trackDownload does not throw (it swallows the error) and that capture was invoked once with the expected payload. Use the existing vi.spyOn/posthog spy pattern and restore/cleanup spies after each test to avoid cross-test pollution; reference the trackDownload function and posthog.capture to find where to add these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/analytics.ts`:
- Around line 68-75: trackDownload is missing the privacy gating and error
handling used elsewhere; update the function so it first calls
analyticsPermittedInitially() and returns early if false, then wrap the
posthog.capture(...) call in a try-catch to swallow/log any exceptions (e.g.,
console.error or existing logger) so download flows aren't interrupted; mirror
the pattern used in getUserLocation and ensure analyticsPermittedInitially is
imported/available in the module.
---
Nitpick comments:
In `@tests/unit/utils/analytics.spec.ts`:
- Around line 93-104: Add two unit tests for trackDownload: one where analytics
is disabled (e.g., set the flag or env/config that trackDownload checks) and
assert posthog.capture is NOT called; and one where posthog.capture is stubbed
to throw and assert that trackDownload does not throw (it swallows the error)
and that capture was invoked once with the expected payload. Use the existing
vi.spyOn/posthog spy pattern and restore/cleanup spies after each test to avoid
cross-test pollution; reference the trackDownload function and posthog.capture
to find where to add these cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18983458-98a5-43cb-8aa0-ce495bfef391
📒 Files selected for processing (7)
src/components/DownloadSelect.vuesrc/components/LocationDownload.vuesrc/components/PrivacyModal.vuesrc/utils/analytics.tstests/unit/components/DownloadSelect.spec.tstests/unit/components/LocationDownload.spec.tstests/unit/utils/analytics.spec.ts
david-mears-2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/analytics.ts`:
- Line 3: Remove the unused import "Dimension" from the top of the module to
satisfy the `@typescript-eslint/no-unused-vars` rule; locate the import statement
importing Dimension (import type { Dimension } from "@/types";) in
src/utils/analytics.ts and delete it (or remove Dimension from the import list
if other types are present).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2375014-d443-4a8a-91cb-0bd3796d2853
📒 Files selected for processing (4)
src/stores/appStore.tssrc/utils/analytics.tstests/unit/stores/appStore.spec.tstests/unit/utils/analytics.spec.ts
✅ Files skipped from review due to trivial changes (1)
- src/stores/appStore.ts
david-mears-2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/stores/appStore.ts`:
- Line 8: The import statement in src/stores/appStore.ts referencing
'@/stores/composables/useAppStoreAnalytics' cannot be resolved (ENOENT); either
add the missing composable module at that path or update the import to the
correct existing file/casing (e.g., match the actual filename under
stores/composables) so Vite can resolve it—locate the import line in appStore.ts
and either create useAppStoreAnalytics.ts with the expected export or change the
import to the correct module name/export.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f62f4dd0-18c1-4a62-9485-2a6a17ce751f
📒 Files selected for processing (6)
src/stores/appStore.tssrc/utils/analytics.tstests/unit/stores/appStore.spec.tstests/unit/utils/analytics.spec.tstests/unit/utils/analyticsDisabled.spec.tstests/unit/utils/analyticsExplicitlyEnabled.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/stores/appStore.spec.ts
- tests/unit/utils/analytics.spec.ts
9c11ea0 to
847365b
Compare
EmmaLRussell
left a comment
EmmaLRussell
left a comment
There was a problem hiding this comment.
Looks great! I wonder if 10s might be slightly too long as a minimum meaningful interaction, but it's in that ballpark.,
For the plot control updates I'd suggest perhaps replacing "true"/"false" for the Split by activity type part with "split"/"no-split" - potentially a little more readable in the posthog dashboard.
What would be the approach if an analytics user wanted to aggregate the analytics e.g. show me all the downloads for India, regardless of user country, or show me all the plot control updates for Cholera, regardless of the other control values? I see that if I export to csv I do get all the values in different columns so I guess that makes it easy to do those kinds of operations outside the dashboard?
I think one tricky consideration for this type of analytic is what to do about default values - it might be that the default value is exactly what the user actively wants and would have chosen if it wasn't already selected, so arguably that should be sent as an analytics event after 10s post-load (but it might also just be that the user is disinterested and not paying attention at all!). If we don't send them, those default values are potentially under-represented in the analytics data.
| downloadErrors.value = []; | ||
| const filenames = files.map((f) => `${f}.csv`); | ||
|
|
||
| trackDownload("summary_table_by_filename", { filenames }); |
There was a problem hiding this comment.
Could you call trackDownload from inside downloadCsvAsSingleOrZip to make it part of the download process, so that we don't need to remember to initially track download on each call to the download method?
There was a problem hiding this comment.
The reason I didn't was because I wanted to track the use of the different UI journeys to downloading: whether the user downloads files by name or by location. It also means that we don't have to pass an extra locations argument to downloadCsvAsSingleOrZip to be forwarded to trackDownload. But probably still worth it for the reason you say.
There was a problem hiding this comment.
The function now looks like:
export const downloadCsvAsSingleOrZip = async (
dataDir: string,
filenames: string[],
zipFileName: string,
locations?: string[]
) => {
if (locations) {
trackDownload("summary_table_by_location", { locations, filenames });
} else {
trackDownload("summary_table_by_filename", { filenames });
}
if (filenames.length === 1 && filenames[0]) {
await downloadSingleFile(dataDir, filenames[0]);
} else if (filenames.length > 1) {
await downloadAsZip(dataDir, filenames, zipFileName);
}
};| if (!analyticsPermittedInitially) return; | ||
| posthog.capture('log_scale_toggle', { | ||
| newScale, | ||
| ...plotProperties, | ||
| }); |
There was a problem hiding this comment.
Should this be delayed to ensure stability too?
There was a problem hiding this comment.
It's only fair.
Yes... I considered '5' as too short, but I hear that there exist some numbers in between - e.g. 8. I will try that one.
I believe for those specific cases you can use the 'filter' feature when creating an 'insight' (a dashboard component): 
Interesting point. I don't know a good solution. I would hope that, if the selections that make the default view are indeed helpful, we would also see views showing up as helpful that vary only slightly from the default, which could still tell us whether the default view is a helpful one (or not as the case may be) if only via reinforcing the individual selections that make it up. |
Also reduce time for tracking plot controls updates - 10 secs was a bit too long
|
I forgot to do this part yesterday:
|
Co-authored-by: Emma Russell <44669576+EmmaLRussell@users.noreply.github.com>
c8d29b3 to
ad55151
Compare
This adds a few more analytics Katy would like to have on vaxviz:
Downloads analytics
Do accounting of download events under a single event type named 'download', but also send information on (1) the type of download, corresponding to whether the download came via the location-based download UI or the other (file-based), (2) if applicable, which locations were selected, and (3) which files were downloaded.
This is sent to posthog, and we can configure the way we want to slice that data there; this can and will be changed, so the analytics sent from vaxviz should not make assumptions about that.
Plot controls
We want to know which views (combinations of plot control selections) are of most interest to users so that we can adjust our controls in response if necessary. Every input change triggers a change in the app store, but that would pollute the events with transient states that were just the user playing around or navigating to their desired state via changing multiple inputs. Thus I only send events once the view has been stable for 10 seconds.
Log scale toggle analytics
Although this is technically a subset of the plot controls event, in order to make it really easy to get a sense of how much this toggle is used, I track this separately, with an event that also records the state of the plot view at the time of toggling the scale from log to linear / vice versa.
Analytics dashboard
Example of how the VIMC dashboard could be configured to consume these analytics events (sent from my localhost): https://eu.posthog.com/project/128512/dashboard/543420
Summary by CodeRabbit
New Features
Documentation
Tests