feat: experiment conversion rate chart - #8463
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
front-end
Docker builds report
|
|
@themis-blindfold review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughAdds typed conversion and exposure timeseries fields to Bayesian results. Adds cumulative and daily conversion-rate data derivation with bucket accumulation and chart metadata. Extends shared charts with configurable stacking, opacity, axes, height, and tooltip formatting. Adds and mounts an experiment conversion-rate card with cumulative/daily display modes and empty-data handling. Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The conversion-rate card and derivation are tested, with no substantiated merge-blocking risk. 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 |
✅ private-cloud · depot-ubuntu-latest-16 — run #20179 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20179 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20179 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20179 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20176 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20176 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20176 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20176 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis review: 🟠 Fix before mergeTL;DR: The cumulative stack currently labels the non-converted remainder as total exposures, and its day-only tooltip keys collide across years. Both cases can show experiment owners incorrect numbers. CI is still in progress; completed unit tests, changed-file lint, and both frontend preview deployments passed.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: This is a solid analysis improvement, but experiment results must not report incorrect exposure counts or mismatched tooltip values. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. May the bars be accurate enough to earn their stack · reviewed at 170e9fc |
Visual Regression19 screenshots compared. See report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: 6bbd7cce-23b2-4fd5-864e-b08998ae2dc2
📒 Files selected for processing (8)
frontend/common/types/responses.tsfrontend/web/components/charts/BarChart.tsxfrontend/web/components/charts/ChartTooltip.tsxfrontend/web/components/experiments/results/ExperimentConversionRateCard/ExperimentConversionRateCard.tsxfrontend/web/components/experiments/results/ExperimentConversionRateCard/index.tsfrontend/web/components/experiments/results/__tests__/deriveConversionRate.test.tsfrontend/web/components/experiments/results/deriveConversionRate.tsfrontend/web/components/pages/ExperimentDetailPage.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
170e9fc to
9811252
Compare
github-actions
Bot
added
feature
|
@themis-blindfold review |
| const opacityMap: Record<string, number> = {} | ||
| const stackMap: Record<string, string> = {} | ||
| identities.forEach((v) => { | ||
| const restKey = `${v.key}${REST_SUFFIX}` |
There was a problem hiding this comment.
🟠 Major · ⚡ Quick win
Synthetic remainder keys collide with valid variant keys.
Observed: variant keys accept slug values and reserve only control, so control__rest is valid. This suffix makes that treatment share the control remainder's key, and the tooltip also recognises it as a remainder. Predicted: an experiment using that treatment key would lose or mislabel both variants' bars and tooltip values. Use collision-proof internal keys for synthetic series and add a regression test.
⚖️ Themis review: 🟠 Fix before mergeTL;DR: The conversion-over-time card handles bucket carry-forward and cross-year labels well, and the completed CI checks are green. A valid treatment key can collide with the generated remainder-series key, so that experiment's chart would misrender its variants.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: A useful experiment-analysis addition, but it must preserve every valid treatment's data before users can rely on it for decisions. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. The chart just needs one private namespace before it meets its public keys · reviewed at 9811252 |
talissoncosta
left a comment
talissoncosta
left a comment
There was a problem hiding this comment.
BarChart picks up seven props here: five that only this card calls, and two (grouped, yAxis) with no caller anywhere.
Pushed a draft on top of your branch instead of a nit list: #8470. One BarSeries[] replaces the four dataKey-keyed maps, which makes grouped unnecessary (side by side is just distinct stack ids, as your daily mode already does) and collapses the two tooltip props into one. 15 props down to 8, your call site from six to two. A PartOfWholeStacks story comes with it, so the stacks and the faded legend are easy to check in Storybook.
Take it or close it, your call. I left REST_SUFFIX and the tooltip's key parsing alone, since they depend on whether buildConversionRateChartData stays.
It targets your branch: cherry-pick the two commits, or land yours first and I'll retarget.
| * Render series side by side instead of stacked. Required for non-additive | ||
| * values (rates, percentages) where stacking would be meaningless. | ||
| */ | ||
| grouped?: boolean |
There was a problem hiding this comment.
It seems to have no caller for this one, and stackMap overrides it anyway. Same for yAxis on line 57. Do you mind checking it ?
| </div> | ||
| ) : undefined | ||
| } | ||
| className='experiment-results__conversion-rate-card' |
There was a problem hiding this comment.
Do you mind checking if this class is defined anywhere? I couldn't find a rule for it, and ContentCard has background='white' if that's the surface you were after.
talissoncosta
left a comment
talissoncosta
left a comment
There was a problem hiding this comment.
Approving. Just left two small comments, not blocking.
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Frontend for the conversion timeseries added in #8451 (branched off it).
Adds a "Conversion rate over time" card to the experiment Analysis section: stacked bars per variant (cumulative exposures, conversions filled in), with a Cumulative/Daily toggle. Daily shows raw per-bucket counts side by side — no per-day rate, per the backend contract.
Derivation lives in a tested
deriveConversionRate.ts;BarChart/ChartTooltipgained optional props only (existing consumers unchanged). The card hides itself on payloads that predate the backend change.How did you test this code?