fix(identities): multivariate override editor hides the identity's override value - #8279
fix(identities): multivariate override editor hides the identity's override value#8279bardock-2393 wants to merge 1 commit into
Conversation
…erride value The editor for a multivariate flag only renders the environment's control value and each variation as radios, so an identity override holding any other value has nowhere to appear. The control row reads as selected and the identity looks like it is on the environment default, even though the SDK still serves the override. Saving the modal then replaced the override with the control value. Show such a value as a read-only, selected row alongside the variations, warn that it is not one of them, and keep it on save unless the user picks the control or a variation instead.
|
@bardock-2393 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
github-actions
Bot
added
the
front-end
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: 0a36622b-b132-4506-adde-3ddfe7686830
📒 Files selected for processing (5)
frontend/common/utils/__tests__/multivariate.test.tsfrontend/common/utils/multivariate.tsfrontend/web/components/modals/create-feature/index.tsxfrontend/web/components/modals/create-feature/tabs/FeatureValueTab.tsxfrontend/web/components/mv/VariationOptions.tsx
| }: { | ||
| controlValue: FlagsmithValue | ||
| overrideValue: FlagsmithValue | ||
| variationOverrides: { percentage_allocation: number }[] | null | undefined |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the variation override union into a named type.
variationOverrides uses an inline union type. Define a named type and use it in the parameter object.
Proposed fix
+type VariationOverrides =
+ | { percentage_allocation: number }[]
+ | null
+ | undefined
+
export const hasUnmatchedIdentityOverride = ({
controlValue,
overrideValue,
variationOverrides,
}: {
controlValue: FlagsmithValue
overrideValue: FlagsmithValue
- variationOverrides: { percentage_allocation: number }[] | null | undefined
+ variationOverrides: VariationOverrides
}): boolean =>As per coding guidelines, frontend/**/*.{ts,tsx} must extract inline union types into named types.
📝 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.
| }: { | |
| controlValue: FlagsmithValue | |
| overrideValue: FlagsmithValue | |
| variationOverrides: { percentage_allocation: number }[] | null | undefined | |
| type VariationOverrides = | |
| | { percentage_allocation: number }[] | |
| | null | |
| | undefined | |
| export const hasUnmatchedIdentityOverride = ({ | |
| controlValue, | |
| overrideValue, | |
| variationOverrides, | |
| }: { | |
| controlValue: FlagsmithValue | |
| overrideValue: FlagsmithValue | |
| variationOverrides: VariationOverrides | |
| }): boolean => |
Source: Coding guidelines
1 participant
When a flag with an existing identity override is later made multivariate, the Edit User Feature modal stops showing what that identity is actually being served. The editor offers only the environment's control value and each variation as radios, so an override holding anything else has nowhere to appear — the control row reads as selected and the identity looks like it is on the environment default. The override is intact and the SDK keeps serving it, but saving the modal replaced it with the control value, so the value could be lost by simply opening and saving.
Rather than hiding it, the editor now shows the override's own value as a read-only, already-selected row alongside the variations, and warns that the value is not one of them so the user can move the identity onto a variation deliberately. Saving keeps the value unless the user picks the control or a variation instead.
Changes
Closes #8271
Review effort: 2/5
How did you test this code?
Manually, end to end, against a local API and dashboard, following the reproduction steps in the issue: a flag with control value
ENV_DEFAULT, an identity override ofMY_OVERRIDE, then a variationVARIANT_Aadded to make the flag multivariate.MY_OVERRIDEselected and read-only,ENV_DEFAULTandVARIANT_Aunselected. Onmainthe same modal showsENV_DEFAULTselected andMY_OVERRIDEnowhere.MY_OVERRIDE(confirmed directly in the database). Onmainthis is where the override was replaced with the control value.Automated: the rule deciding whether an override is representable by the variation radios is unit tested in
common/utils/__tests__/multivariate.test.ts, including the plan-unchanged case, an identity assigned a variation, a partially weighted override, and null/undefined/empty edge cases.npm run test:unitpasses in full (402 tests),npm run lintreports nothing on the changed files, andnpm run typecheckproduces an identical error set tomainfor them.A note on behaviour
The same guard applies when a multivariate identity override's stored value has drifted from the environment's control value for any other reason — for example the environment control being edited after the override was made. Previously a save silently re-synced the identity onto the new control value, changing what that identity is served; now the value is shown, flagged, and left alone until the user chooses. That seemed the safer default given the issue is about a value disappearing, but say the word if you would rather keep re-syncing in that case.