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 3 commits 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 Merge Risk: 🔵 Low · up to The change prevents an identity override from being silently replaced when editing a multivariate flag, but the new editor behavior still has bounded frontend risks around keyboard accessibility and state handling that should have explicit owner follow-up before or after merge. ✨ 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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hey @bardock-2393, looks like there are some automated review comments to address. Thanks for your contribution! |
|
I will resolved it today |
|
hey @bardock-2393 thanks for raising this use case, what you've done makes sense however I think one thing could do with fixing: 
Here as soon as I click the second option the first (the identity override) vanishes. I think it should only be removed once the feature has been saved. Thanks! |
Picking a variation removed the unmatched override row outright, because its presence was read from the same live editor state that decides which row is selected. The value the user is choosing to replace vanished from under them, with no way back short of closing the modal. Latch the row's presence for the lifetime of the editor, and let only its selected state follow the live edits. Selecting it again restores the original value, so the row is a real choice rather than a notice.
The rule the previous commit fixed — presence outlives selection — lived inline in the tab, where nothing could reach it: jest runs in a node environment with no DOM, so a component test would mean new dependencies. Lift the decision into resolveUnmatchedOverride and test it there. The three cases that matter fail against the old presence-follows-selection rule, including the null override value that a naive latch drops.
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: 9f96d5c8-43ac-4ac5-8883-9d6c5e5ffe6c
📒 Files selected for processing (3)
frontend/common/utils/multivariate.tsfrontend/web/components/modals/create-feature/tabs/FeatureValueTab.tsxfrontend/web/components/mv/VariationOptions.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| <div | ||
| data-test='select-unmatched-override' | ||
| onMouseDown={(e) => { | ||
| e.stopPropagation() | ||
| setVariations([]) | ||
| setValue?.(unmatchedOverride.value) | ||
| }} | ||
| className={`btn-radio ml-2 ${ | ||
| unmatchedOverride.selected ? 'btn-radio-on' : '' | ||
| }`} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the unmatched override selector keyboard accessible.
This interactive div cannot receive keyboard focus or keyboard activation. Keyboard users cannot select the unmatched override, so they cannot restore its preserved value.
Use a native button or implement radio semantics, focus handling, aria-checked, and Enter/Space activation.
There was a problem hiding this comment.
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/modals/create-feature/tabs/FeatureValueTab.tsx (1)
446-448: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the warning visible while the unmatched row is latched.
Line 446 checks
unmatchedOverrideSelected. This becomes false after the user selects the control value or a variation, whileunmatchedOverrideremains visible. CheckunmatchedOverrideso the warning remains with the read-only unmatched row until save.Proposed fix
- {unmatchedOverrideSelected && ( + {unmatchedOverride && ( <WarningMessage warningMessage="This identity override contains a value that is not one of this flag's variations. We recommend changing it." /> )}
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1806800-fcc3-4a8f-b77d-71ddd6509499
📒 Files selected for processing (3)
frontend/common/utils/__tests__/multivariate.test.tsfrontend/common/utils/multivariate.tsfrontend/web/components/modals/create-feature/tabs/FeatureValueTab.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| // Held in a ref rather than read once on mount, as the feature state loads | ||
| // async — there is no single render at which the override is known to be | ||
| // there. See resolveUnmatchedOverride for why presence outlives selection. | ||
| const latchedOverrideValue = useRef<FlagsmithValue | undefined>(undefined) | ||
| const unmatchedOverride = resolveUnmatchedOverride({ | ||
| isSelected: unmatchedOverrideSelected, | ||
| latchedValue: latchedOverrideValue.current, | ||
| overrideValue: featureState.feature_state_value ?? null, | ||
| }) | ||
| latchedOverrideValue.current = unmatchedOverride?.value |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="frontend/web/components/modals/create-feature/tabs/FeatureValueTab.tsx"
# Inspect render-phase ref writes and commit-phase synchronisation.
rg -n -C 5 'latchedOverrideValue\.current|useEffect\(' "$target"
# Inspect configured React Hooks or React Compiler rules.
fd -a -t f 'eslint.config.*' . | while IFS= read -r file; do
rg -n -C 3 'react-hooks|react-compiler|refs' "$file" || true
doneRepository: Flagsmith/flagsmith
Length of output: 1203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="frontend/web/components/modals/create-feature/tabs/FeatureValueTab.tsx"
printf '%s\n' '--- imports and relevant component body ---'
sed -n '1,190p' "$target"
sed -n '280,470p' "$target"
printf '%s\n' '--- resolver definition and call sites ---'
rg -n -C 12 'resolveUnmatchedOverride|unmatchedOverrideSelected|latchedOverrideValue' frontend common 2>/dev/null || true
printf '%s\n' '--- hook and compiler configuration ---'
fd -a -t f . | rg '(^|/)(eslint|biome|oxlint|package\.json|tsconfig).*' |
while IFS= read -r file; do
rg -n -C 3 'react-hooks|react-compiler|refs|purity|set-state-in-render' "$file" || true
doneRepository: Flagsmith/flagsmith
Length of output: 35555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- React versions and ref-write patterns ---'
rg -n '"react"|"react-dom"|"eslint-plugin-react-hooks"|refs|set-state-in-render' \
package.json frontend/**/package.json .eslintrc* eslint.config.* 2>/dev/null || true
rg -n -U 'useRef<[^;]+>\([^;]*\)[\s\S]{0,500}\.current\s*=' frontend common \
-g '*.{ts,tsx,js,jsx}' 2>/dev/null | head -200 || true
printf '%s\n' '--- resolver implementation and tests ---'
sed -n '1,75p' frontend/common/utils/multivariate.ts
sed -n '119,198p' frontend/common/utils/__tests__/multivariate.test.ts
printf '%s\n' '--- deterministic render/effect model ---'
python3 - <<'PY'
UNSET = object()
def resolve(is_selected, latched, override):
value = override if latched is UNSET and is_selected else latched
return None if value is UNSET else (is_selected, value)
def render_phase(sequence):
latched = UNSET
committed = None
for label, selected, override, commit in sequence:
result = resolve(selected, latched, override)
latched = result[1] if result is not None else UNSET
if commit:
committed = result
print(label, 'committed' if commit else 'aborted',
'result=', result, 'ref=', latched)
print('render-phase final committed=', committed, 'ref=', latched)
def effect_phase(sequence):
latched = UNSET
committed = None
pending_effect = None
for label, selected, override, commit in sequence:
result = resolve(selected, latched, override)
pending_effect = None if result is None else result[1]
if commit:
committed = result
latched = pending_effect
print(label, 'committed' if commit else 'aborted',
'result=', result, 'ref=', latched,
'effect=', pending_effect if commit else 'not run')
print('effect-phase final committed=', committed, 'ref=', latched)
sequence = [
('async data absent', False, None, True),
('override appears', True, 'OLD', False),
('later committed data', True, 'NEW', True),
('user selects variation', False, 'NEW', True),
]
print('Case: an abandoned render occurs before the first committed override render')
render_phase(sequence)
effect_phase(sequence)
PYRepository: Flagsmith/flagsmith
Length of output: 23111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package React versions ---'
rg -n -C 2 '"react"|"react-dom"|"react-test-renderer"' package.json frontend -g 'package.json' 2>/dev/null || true
printf '%s\n' '--- corrected concurrent-render model ---'
python3 - <<'PY'
UNSET = object()
def resolve(is_selected, latched, override):
value = override if latched is UNSET and is_selected else latched
return None if value is UNSET else (is_selected, value)
def render_latch(sequence):
latched = UNSET
committed_result = None
for label, selected, override, commits in sequence:
result = resolve(selected, latched, override)
# This is the implementation under review: the write occurs even if
# this render is later abandoned.
latched = UNSET if result is None else result[1]
if commits:
committed_result = result
print(f'{label}: {"commit" if commits else "abort"}, '
f'result={result}, ref={None if latched is UNSET else latched!r}')
return committed_result, latched
def effect_latch(sequence):
latched = UNSET
committed_result = None
for label, selected, override, commits in sequence:
result = resolve(selected, latched, override)
# An effect runs only for a committed render and writes undefined when
# the result is absent, preserving the unset sentinel.
if commits:
committed_result = result
latched = UNSET if result is None else result[1]
print(f'{label}: {"commit" if commits else "abort"}, '
f'result={result}, ref={None if latched is UNSET else latched!r}')
return committed_result, latched
sequence = [
('initial data absent', False, None, True),
('abandoned render sees OLD override', True, 'OLD', False),
('committed render sees NEW override', True, 'NEW', True),
('user selects a variation', False, 'NEW', True),
]
print('render-phase write:')
print('final:', render_latch(sequence))
print('commit-phase write:')
print('final:', effect_latch(sequence))
PYRepository: Flagsmith/flagsmith
Length of output: 1761
Move the ref write out of render.
Line 313 mutates latchedOverrideValue.current during rendering. An abandoned concurrent render can leave this mutation in place. A later committed render can then show a stale unmatched override. Update the latch in a commit-phase effect.
Proposed fix
const unmatchedOverride = resolveUnmatchedOverride({
isSelected: unmatchedOverrideSelected,
latchedValue: latchedOverrideValue.current,
overrideValue: featureState.feature_state_value ?? null,
})
-latchedOverrideValue.current = unmatchedOverride?.value
+useEffect(() => {
+ latchedOverrideValue.current = unmatchedOverride?.value
+}, [unmatchedOverride?.value])📝 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.
| // Held in a ref rather than read once on mount, as the feature state loads | |
| // async — there is no single render at which the override is known to be | |
| // there. See resolveUnmatchedOverride for why presence outlives selection. | |
| const latchedOverrideValue = useRef<FlagsmithValue | undefined>(undefined) | |
| const unmatchedOverride = resolveUnmatchedOverride({ | |
| isSelected: unmatchedOverrideSelected, | |
| latchedValue: latchedOverrideValue.current, | |
| overrideValue: featureState.feature_state_value ?? null, | |
| }) | |
| latchedOverrideValue.current = unmatchedOverride?.value | |
| // Held in a ref rather than read once on mount, as the feature state loads | |
| // async — there is no single render at which the override is known to be | |
| // there. See resolveUnmatchedOverride for why presence outlives selection. | |
| const latchedOverrideValue = useRef<FlagsmithValue | undefined>(undefined) | |
| const unmatchedOverride = resolveUnmatchedOverride({ | |
| isSelected: unmatchedOverrideSelected, | |
| latchedValue: latchedOverrideValue.current, | |
| overrideValue: featureState.feature_state_value ?? null, | |
| }) | |
| useEffect(() => { | |
| latchedOverrideValue.current = unmatchedOverride?.value | |
| }, [unmatchedOverride?.value]) |
3 participants
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.