refactor(analytics): make trackEvent error-safe, add set-once trait helper#8061
refactor(analytics): make trackEvent error-safe, add set-once trait helper#8061talissoncosta wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
da1ace9 to
c67bc32
Compare
Use amplitude setOnce for onboarding_variant so it's pinned to the user's first exposure and doesn't get overwritten on later loads. Adds API.trackTraitsOnce; the getting-started gate uses it. Addresses Wadii's review note on #7960. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a991946 to
7442688
Compare
A failing tracker (GA/amplitude) can't break the caller, e.g. the onboarding bootstrap firing milestone events. Addresses kyle's note on #7960. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-16 — run #18629 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18629 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18629 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18629 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18578 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18578 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18578 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18578 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
⚖️ Themis judgement: 🧹 Ship it, nits insideTwo small, targeted robustness fixes for the analytics helpers introduced in #7960:
🧹 Nits
📝 Walkthrough
🧪 How to verify
Automate: A unit test for Product take: Solid quality-of-life improvement for analytics reliability. Pinning the onboarding variant with 🧭 Assumptions & unverified claims
A try/catch and a setOnce walk into a bar — one catches everything, the other remembers only the first drink. · reviewed at 1707416 |
|
@themis-blindfold is there still anything left to address from the #7960 pull request, related to the use of the feature flag ? |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9bce5b1b-ba3d-43fa-8949-7a30bd203294
📒 Files selected for processing (2)
frontend/web/components/pages/onboarding/GettingStartedGate.tsxfrontend/web/project/api.ts
|
I don't have access to the #7960 conversation, but the PR description for this PR (#8061) explicitly names two deferred feature-flag items from #7960. Both are resolved in the current code:
The feature-flag wiring itself (
If there were other flag-related items raised on #7960 beyond those two, I'd need the original thread to check — but the two items this PR was opened to address are both resolved. |
Match the try/catch just added to trackEvent (and the existing identify helper): amplitude.identify() is called from a useEffect in GettingStartedGate, so a failing tracker shouldn't propagate into the component lifecycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to #7960. Two robustness fixes to the shared analytics helpers, deferred to keep #7960 onboarding-only.
Changes
trackTraitsOnce(amplitudesetOnce) so the onboarding variant is pinned to a user's first exposure and can't be overwritten on later loads (Wadii's note). The getting-started gate uses it.trackEventwrapped in try/catch so a failing tracker can't break the caller, e.g. the onboarding bootstrap firing milestone events (kyle's note).How did you test this code?
Sign up, reach onboarding, confirm
onboarding_variantis set once and unchanged on revisit, and events still fire.