feat(experiments): colour lift by metric direction#8099
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughExperiment metric responses now include Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
Docker builds report
|
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: 672c9bc6-7802-49be-af67-0f1b14658fea
📒 Files selected for processing (7)
frontend/common/types/responses.tsfrontend/web/components/experiments/results/ExperimentMetricScorecard.tsxfrontend/web/components/experiments/results/ExperimentResultsAxisChart.tsxfrontend/web/components/experiments/results/ExperimentResultsScorecardTable.tsxfrontend/web/components/experiments/results/ExperimentSummaryScorecard.tsxfrontend/web/components/experiments/results/__tests__/derive.test.tsfrontend/web/components/experiments/results/derive.ts
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18712 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #18712 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18712 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18712 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18709 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18709 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18709 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #18709 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2086e0cc-6225-41ee-b6e4-ada54e22c2c9
📒 Files selected for processing (5)
frontend/common/types/responses.tsfrontend/web/components/experiments/results/ExperimentMetricScorecard.tsxfrontend/web/components/experiments/results/ExperimentResultsScorecardTable.tsxfrontend/web/components/experiments/results/__tests__/derive.test.tsfrontend/web/components/experiments/results/derive.ts
| it.each<[MetricDirection | undefined, LiftTone]>([ | ||
| ['up', 'success'], | ||
| ['down', 'danger'], | ||
| ['informational', 'neutral'], | ||
| [undefined, 'success'], // legacy payloads without direction default to up |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Extract the optional direction union into a named type.
MetricDirection | undefined is declared inline in the it.each generic. Define a named alias, such as OptionalMetricDirection, and use it here.
As per coding guidelines, TypeScript union types in frontend/**/*.{ts,tsx} must be extracted into named types.
Proposed adjustment
+type OptionalMetricDirection = MetricDirection | undefined
+
-it.each<[MetricDirection | undefined, LiftTone]>([
+it.each<[OptionalMetricDirection, LiftTone]>([📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.each<[MetricDirection | undefined, LiftTone]>([ | |
| ['up', 'success'], | |
| ['down', 'danger'], | |
| ['informational', 'neutral'], | |
| [undefined, 'success'], // legacy payloads without direction default to up | |
| type OptionalMetricDirection = MetricDirection | undefined | |
| it.each<[OptionalMetricDirection, LiftTone]>([ | |
| ['up', 'success'], | |
| ['down', 'danger'], | |
| ['informational', 'neutral'], | |
| [undefined, 'success'], // legacy payloads without direction default to up |
Source: Coding guidelines
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Depends on #8098.
Colours experiment lift values by the primary metric's direction instead of sign only (follow-up to #8091):
liftToneof success/danger/neutral).directionis absent from the API response and metrics fall back to "higher is better" — identical to current behaviour.How did you test this code?
Parametrised unit tests for direction → tone mapping in
derive.test.ts; manually checked chart/table/scorecard colours for up and down metrics.