Skip to content

chore: delete old strategy variants; use new variants in default strategy #11343

Closed
thomasheartman wants to merge 1 commit intomainfrom
heart/push-wotpylvuktxq
Closed

chore: delete old strategy variants; use new variants in default strategy #11343
thomasheartman wants to merge 1 commit intomainfrom
heart/push-wotpylvuktxq

Conversation

@thomasheartman
Copy link
Copy Markdown
Contributor

@thomasheartman thomasheartman commented Feb 17, 2026

Instead of the old variants, let's use the new variants for the default strategy.

From manual testing, they appear to work almost the exact same. My guess is that the old version was just forgotten after #5850.

The permissions that we used to require are the same permissions as the ones required to save the form, so there's no need to but those permissions on the "add variants" button.

In all the following screenies, the new strategy variants version is shown on the left, the old version on the right:

No variants:
image

Open editing:
image

Custom weighting:
image

Without names:
image

with names:
image

saved:
image

Claude's summary:

Here are the key differences between the two components:

1. Initialization pattern

StrategyVariants initializes variants in a useEffect with empty dependency array, setting state after mount. NewStrategyVariants initializes variants directly in the useState call, avoiding the extra render cycle.

2. Cleanup effect

NewStrategyVariants has an additional cleanup useEffect (return function) that filters out variants with empty names when the component unmounts. StrategyVariants has no such cleanup.

3. Weight validation

NewStrategyVariants computes a variantWeightsError boolean (checks if weights don't sum to 1000) and passes it to both VariantForm (as weightsError) and VariantsSplitPreview (as weightsError). StrategyVariants does no weight validation.

4. Permission prop

StrategyVariants accepts a configurable permission prop (defaulting to UPDATE_FEATURE_ENVIRONMENT_VARIANTS). NewStrategyVariants hardcodes UPDATE_FEATURE_ENVIRONMENT_VARIANTS directly.

5. UI differences

  • NewStrategyVariants shows an Alert info banner at the top explaining what variants do.
  • The help tooltip text is longer and more descriptive in NewStrategyVariants.
  • The help icon uses a styled Box wrapper (StyledHelpIconBox) instead of being inline in the h3.
  • The "Add variant" button has a startIcon={<Add />} icon in NewStrategyVariants.
  • StrategyVariants renders the heading as an <h3> via Typography variant='h3'; NewStrategyVariants uses a plain Typography (no heading variant).
  • The StrategyVariantsUpgradeAlert is conditionally rendered only when variants exist in NewStrategyVariants, but always shown in StrategyVariants.
  • NewStrategyVariants uses a plain <a> tag for the docs link; StrategyVariants uses MUI's <Link>.

In summary: NewStrategyVariants is a revised version with better initialization (no unnecessary re-render), weight validation, cleanup on unmount, and a refreshed UI. StrategyVariants is the older version with more flexibility (configurable permission) but fewer guardrails.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 17, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This adds the parameterized permission back in, defaulting to what was the hardcoded value before. It's likely that the implementation will change a bit when merging it with milestone strategy variants, so this is likely to be a stopgap.

@thomasheartman thomasheartman changed the title chore: delete old strategy variants chore: delete old strategy variants; use new variants in default strategy Feb 17, 2026
@gastonfournier gastonfournier moved this from New to In Progress in Issues and PRs Feb 18, 2026
@thomasheartman thomasheartman force-pushed the heart/push-wotpylvuktxq branch 3 times, most recently from be36eaf to 1949f70 Compare February 18, 2026 10:49
@thomasheartman thomasheartman force-pushed the heart/push-wotpylvuktxq branch from 1949f70 to ee313d2 Compare February 18, 2026 10:50
Comment on lines -187 to -190
permission={[
PROJECT_DEFAULT_STRATEGY_WRITE,
UPDATE_PROJECT,
]}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These permissions are already required to save the form itself. It doesn't really make any sense to gate this one form field behind permissions you already need.

@thomasheartman thomasheartman added the 👤🤖 mostly-human Human-led with LLM assistance label Feb 25, 2026
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Issues and PRs Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

👤🤖 mostly-human Human-led with LLM assistance

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants