Skip to content

fix(experiments): results page control-wins state, lift colours, dark mode#8091

Merged
Zaimwa9 merged 3 commits into
mainfrom
fix/experiment-results-control-wins
Jul 24, 2026
Merged

fix(experiments): results page control-wins state, lift colours, dark mode#8091
Zaimwa9 merged 3 commits into
mainfrom
fix/experiment-results-control-wins

Conversation

@Zaimwa9

@Zaimwa9 Zaimwa9 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Thanks for submitting a PR! Please check the boxes below:

  • I have read the Contributing Guide.
  • I have added information to docs/ if required so people know about the feature.
  • I have filled in the "Changes" section below.
  • I have filled in the "How did you test this code" section below.

Changes

UI fixes for the experiment results page:

  • Control can now win: when no variant is likely to beat it, the banner turns into a warning ("Control is the winner"), the winning variation card shows Control, and lift shows "Baseline".
  • Lift colours are sign-based only (positive green, negative red) — expected_direction no longer affects colouring.
  • Fixed unreadable variant names in the banner in dark mode (.alert-success strong had no dark override).
  • Hypothesis: standard title colour, edit button next to the title, full-width text.

How did you test this code?

Unit tests for the winner/summary derivation; manually checked the control-wins state, negative lift colours, and dark mode.

Before
image

After
image

@Zaimwa9
Zaimwa9 requested a review from a team as a code owner July 24, 2026 08:15
@Zaimwa9
Zaimwa9 requested review from talissoncosta and removed request for a team July 24, 2026 08:15
@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
flagsmith-frontend-preview Ready Ready Preview, Comment Jul 24, 2026 9:17am
flagsmith-frontend-staging Ready Ready Preview, Comment Jul 24, 2026 9:17am
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Ignored Ignored Preview Jul 24, 2026 9:17am

Request Review

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Experiment result derivation now compares treatment probabilities with a conservative control probability and can identify the baseline as the winner. Shared summary logic is used by scorecards and recommendations, while lift colouring depends only on lift sign. Control-win states receive distinct rendering. Hypothesis layout and dark-theme alert styling are also adjusted, with new derivation and summary test coverage.

Estimated code review effort: 3 (Moderate) | ~25 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Docker builds report

Image Build Status Security report
ghcr.io/flagsmith/flagsmith-api-test:pr-8091 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-e2e:pr-8091 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api:pr-8091 Finished ✅ Results
ghcr.io/flagsmith/flagsmith:pr-8091 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-private-cloud:pr-8091 Finished ✅ Results

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18694 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)

passed  3 passed

Details

stats  3 tests across 3 suites
duration  36.3 seconds
commit  9c9ce3f
info  🔄 Run: #18694 (attempt 1)

🗂️ Previous results
✅ private-cloud · depot-ubuntu-latest-16 — run #18694 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-16)

passed  3 passed

Details

stats  3 tests across 3 suites
duration  32.4 seconds
commit  9c9ce3f
info  🔄 Run: #18694 (attempt 1)

✅ oss · depot-ubuntu-latest-16 — run #18694 (attempt 1)

Playwright Test Results (oss - depot-ubuntu-latest-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  38.1 seconds
commit  9c9ce3f
info  🔄 Run: #18694 (attempt 1)

✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18690 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  1 minute, 11 seconds
commit  5e8066b
info  🔄 Run: #18690 (attempt 1)

✅ private-cloud · depot-ubuntu-latest-16 — run #18690 (attempt 1)

Playwright Test Results (private-cloud - depot-ubuntu-latest-16)

passed  2 passed

Details

stats  2 tests across 2 suites
duration  48.6 seconds
commit  5e8066b
info  🔄 Run: #18690 (attempt 1)

✅ oss · depot-ubuntu-latest-arm-16 — run #18690 (attempt 1)

Playwright Test Results (oss - depot-ubuntu-latest-arm-16)

passed  1 passed

Details

stats  1 test across 1 suite
duration  42.7 seconds
commit  5e8066b
info  🔄 Run: #18690 (attempt 1)

✅ oss · depot-ubuntu-latest-16 — run #18690 (attempt 1)

Playwright Test Results (oss - depot-ubuntu-latest-16)

passed  2 passed

Details

stats  2 tests across 2 suites
duration  41.6 seconds
commit  5e8066b
info  🔄 Run: #18690 (attempt 1)

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Visual Regression

19 screenshots compared. See report for details.
View full report

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 169bd11e-872a-421e-b5c1-f5171eeee9cb

📥 Commits

Reviewing files that changed from the base of the PR and between 30bf03a and 5e8066b.

📒 Files selected for processing (9)
  • frontend/web/components/experiments/results/ExperimentDetailHeader.tsx
  • frontend/web/components/experiments/results/ExperimentMetricScorecard.tsx
  • frontend/web/components/experiments/results/ExperimentRecommendation.tsx
  • frontend/web/components/experiments/results/ExperimentResultsAxisChart.tsx
  • frontend/web/components/experiments/results/ExperimentResultsScorecardTable.tsx
  • frontend/web/components/experiments/results/ExperimentSummaryScorecard.tsx
  • frontend/web/components/experiments/results/__tests__/derive.test.ts
  • frontend/web/components/experiments/results/derive.ts
  • frontend/web/styles/project/_alert.scss
💤 Files with no reviewable changes (1)
  • frontend/web/components/experiments/results/ExperimentMetricScorecard.tsx

Comment thread frontend/web/components/experiments/results/ExperimentResultsAxisChart.tsx Outdated
Comment thread frontend/web/styles/project/_alert.scss
@Zaimwa9

Zaimwa9 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@themis-blindfold review

@themis-blindfold

Copy link
Copy Markdown
Contributor

⚖️ Themis judgement: ✅ Ship it

Clean feature PR that adds a control-wins state to the experiment results page, simplifies lift colouring to sign-only, and fixes dark mode readability for the success alert banner. The core logic in getWinningVariant is correct — the Bonferroni bound for control's chance is well-documented and the edge cases (no inference, sum > 1) are handled. deriveSummary moves to derive.ts where it belongs and picks up proper test coverage. CI is fully green across all checks.

One gap I independently verified: the new .alert-warning banner (control-wins path) renders VariantName which uses <strong>, but .dark .alert-warning only overrides background-color — no strong colour override. The <strong> text inherits $alert-color-dark (48% white), making variant names dimmer than the fully-opaque-white treatment in the success banner's new strong override. This is already raised in an existing review thread and is cosmetic rather than blocking.

🎯 Correctness 4/5
🧪 Test coverage 5/5
📐 Code quality 4/5
🚀 Product impact 3/5
📝 Walkthrough
  • derive.tsisLiftFavourable simplified to sign-only (no expected_direction); getWinningVariant extended to return control when no treatment beats it; deriveSummary moved here from ExperimentSummaryScorecard and enriched with controlWins flag.
  • ExperimentRecommendation — new control-wins branch renders an alert-warning banner with recommendation to roll out Control.
  • ExperimentSummaryScorecard — consumes deriveSummary from derive.ts; winner name and lift styling now conditional on controlWins.
  • ExperimentResultsAxisChart / ExperimentResultsScorecardTabledirection prop and ExpectedDirection references removed; getLiftColour simplified.
  • _alert.scss — dark mode strong/.title colour overrides added for .alert-success.
  • derive.test.ts — new test suites for getWinningVariant and deriveSummary covering treatment-wins, control-wins, and empty-inference scenarios.
🧪 How to verify
  1. Set up an experiment where all treatments have low chance_to_win (< 0.5 each, sum < 0.5) — the recommendation banner should show alert-warning "Control is the winner" and the scorecard should display "Control" as winner with "Baseline" lift.
  2. Set up an experiment where one treatment has high chance_to_win — the banner should remain alert-success with the treatment name and positive lift.
  3. Toggle dark mode in both scenarios; confirm variant names are readable in both banners. Note the <strong> text in the warning banner may appear dimmer — this is the known gap.
  4. Run npx jest --testPathPattern derive.test to confirm all unit tests pass.
  5. Verify that lift colours are strictly sign-based (positive = green, negative = red) regardless of expected_direction.

Automate: The new derive tests cover the logic; a Chromatic or visual regression snapshot of the warning banner in dark mode would lock in the contrast fix once the strong override is added.

Product take: Solid improvement — experiments that confirm the control is winning now get a clear signal instead of a confusing absence of recommendation. Simplifying lift colours to sign-only is the right call while expected_direction data is unreliable; it removes a source of misleading colouring. Minor polish, meaningful UX win for experiment operators.

🧭 Assumptions & unverified claims
  • The code comment states expected_direction "is not used reliably yet" — the review takes this at face value as a known product constraint. If expected_direction is actually reliable for some metric types, the sign-only colouring would mislead for decrease-is-good metrics (e.g. error rates).
  • The Bonferroni bound for control's win probability (1 - Σ chance_to_win) is documented as conservative for > 1 treatment. With many low-probability treatments this may slightly overstate control's chance, but the comment acknowledges this and the UI is advisory, not a decision gate.

Control wins this round — and now the UI knows it too. · reviewed at 5e8066b

@Zaimwa9

Zaimwa9 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Reviewers comparison

  • Accuracy: Themis's single finding (warning-banner <strong> inherits 48%-white $alert-color-dark in dark mode) was correct and precisely explained. CodeRabbit pointed at the same area but overstated it as unreadable and suggested a redundant fix (.text-warning is already token-based).
  • Calibration: Themis graded it cosmetic/non-blocking — right call. CodeRabbit inverted severities: a non-issue flagged Major (the fw-semibold SuccessMessage title, unaffected by this PR), a real-but-mild issue flagged Minor.
  • Coverage: CodeRabbit's per-line sweep caught the one thing Themis missed — the duplicated getLiftColour helper (now deduped).
  • Added value: Themis's "assumptions & unverified claims" section flagged exactly the judgement calls behind this PR (direction reliability, Bonferroni bound) instead of reporting them as defects; its verification steps were runnable.
  • Net: Themis = fewer, correct, well-prioritised findings; CodeRabbit = broader sweep with useful trivia but noisier severity labels. Both actionable items landed in 9c9ce3f.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/web/components/experiments/results/derive.ts (1)

157-176: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not treat missing treatment inference as control probability.

Lines 160-162 skip treatments without inference, then Lines 175-176 assign the remaining probability mass to Control. Because missing inference is a supported state, this can falsely select Control when a treatment is still collecting data. Return an insufficient-data result unless every treatment has inference before calculating the residual control probability.

Proposed fix
   let best: WinningVariant | null = null
   let treatmentChancesTotal = 0
+  if (
+    identities.some(
+      (variant) => !variant.isControl && !metricResult.inference[variant.key],
+    )
+  ) {
+    return null
+  }
   for (const v of identities) {
     if (v.isControl) continue
     const inf = metricResult.inference[v.key]
-    if (!inf) continue
     treatmentChancesTotal += inf.chance_to_win

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a150032f-1e68-4bc0-8731-412b0d98578a

📥 Commits

Reviewing files that changed from the base of the PR and between 5e8066b and 9c9ce3f.

📒 Files selected for processing (4)
  • frontend/web/components/experiments/results/ExperimentResultsAxisChart.tsx
  • frontend/web/components/experiments/results/ExperimentResultsScorecardTable.tsx
  • frontend/web/components/experiments/results/derive.ts
  • frontend/web/styles/project/_alert.scss

@Zaimwa9
Zaimwa9 merged commit 44b2464 into main Jul 24, 2026
29 checks passed
@Zaimwa9
Zaimwa9 deleted the fix/experiment-results-control-wins branch July 24, 2026 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix front-end Issue related to the React Front End Dashboard

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants