feat(mv): render variant keys in mono - #8385
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdded the exported Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to This change only updates how variant keys are presented. The remaining bounded risk is repository consistency and maintainability: add the required component stylesheet, name the nullable value type, and use the configured import alias. No concrete user or production correctness risk is identified, so it is mergeable with owner follow-up. 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
A variant key is an identifier, not prose. It is validated as a slug, and util/mappers/engine.py carries it into the environment document, so local evaluation SDKs match on it. Rendering it in body type hides the differences that matter when someone transcribes it into code: 0 against O, l against 1, a stray trailing character. Adds a VariantKey component that resolves the key or its Variant_n fallback and sets it in mono, then uses it at the four surfaces that show one: the identity override selector, the feature editor label, and the two experiments views. The fallback was duplicated at each of them. Uses Bootstrap's font-monospace, which this build already generates. A new token would have been a third mono stack in the tree, disagreeing with Bootstrap's over SFMono-Regular. Settling that is its own change. Copyable is left for a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1af29d1 to
a69c3aa
Compare
github-actions
Bot
added
feature
tsconfig sets jsx: react-jsx, so the automatic runtime covers it. Matches VariationTable next door. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 134b5d25-fce9-428e-9ecc-4317acda6dfb
📒 Files selected for processing (6)
frontend/web/components/experiments/RolloutSplitEditor/RolloutSplitEditor.tsxfrontend/web/components/experiments/VariationTable/VariationTable.tsxfrontend/web/components/mv/VariantKey/VariantKey.tsxfrontend/web/components/mv/VariantKey/index.tsfrontend/web/components/mv/VariationKeyLabel/VariationKeyLabel.tsxfrontend/web/components/mv/VariationOptions.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| interface VariantKeyProps | ||
| extends Omit<ComponentPropsWithoutRef<'span'>, 'children'> { | ||
| // The variant's own key, absent when the user never set one. | ||
| value?: string | null | ||
| index: number |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the nullable value type into a named type.
VariantKeyProps.value declares an inline union. Define a named type and use it for the property.
Proposed change
+type VariantKeyValue = string | null
+
interface VariantKeyProps
extends Omit<ComponentPropsWithoutRef<'span'>, 'children'> {
- value?: string | null
+ value?: VariantKeyValueAs per coding guidelines, TypeScript files must use named union 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.
| interface VariantKeyProps | |
| extends Omit<ComponentPropsWithoutRef<'span'>, 'children'> { | |
| // The variant's own key, absent when the user never set one. | |
| value?: string | null | |
| index: number | |
| type VariantKeyValue = string | null | |
| interface VariantKeyProps | |
| extends Omit<ComponentPropsWithoutRef<'span'>, 'children'> { | |
| // The variant's own key, absent when the user never set one. | |
| value?: VariantKeyValue | |
| index: number |
Source: Coding guidelines
| export const VariantKey: FC<VariantKeyProps> = ({ | ||
| className, | ||
| index, | ||
| value, | ||
| ...rest | ||
| }) => ( | ||
| <span className={classNames('font-monospace', className)} {...rest}> | ||
| {value || getDefaultVariantKey(index)} | ||
| </span> | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required co-located stylesheet.
The new VariantKey component folder contains no VariantKey.scss file. Add the co-located stylesheet required for new components before merging.
As per coding guidelines, each new component must include a co-located ComponentName.scss file.
Source: Coding guidelines
| import Utils from 'common/utils/utils' | ||
| import { FlagsmithValue, MultivariateOption } from 'common/types/responses' | ||
| import { UnmatchedOverride } from 'common/utils/multivariate' | ||
| import { VariantKey } from './VariantKey' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the configured component alias.
This frontend consumer uses a relative import. Import VariantKey through components/mv/VariantKey.
Proposed change
-import { VariantKey } from './VariantKey'
+import { VariantKey } from 'components/mv/VariantKey'As per coding guidelines, frontend files must use common/, components/, or project/ import paths instead of relative imports.
📝 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.
| import { VariantKey } from './VariantKey' | |
| import { VariantKey } from 'components/mv/VariantKey' |
Source: Coding guidelines
github-actions
Bot
added
feature
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19855 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #19855 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19855 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19855 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Follows @khvn26's suggestion on #8372, that variant keys read as prose when they are actually identifiers.
The premise holds:
MultivariateFeatureOption.keyis declared as "A stable, human-readable identifier for the variant", is validated withvalidate_slug, andutil/mappers/engine.py:189carries it into the environment document, so local evaluation SDKs match on it. Body type hides the differences that matter when someone transcribes one into code:0againstO,lagainst1, a stray trailing character.VariantKeycomponent. Resolveskeyor itsVariant_nfallback and sets it in mono. That fallback was duplicated at every call site.VariationTableandRolloutSplitEditor.rollout.ts:93builds a key into a plain label string for a chart legend, so it is untouched.Uses Bootstrap's
.font-monospace, which this build already generates:3rdParty/_bootstrap.scss:12imports~bootstrap/scss/rootso--bs-font-monospaceis defined, line 71 imports the utilities API, and the map-removals below it only touchcolor,border-colorandshadow, so thefont-familykey survives.Deliberately not here
A
--font-family-monotoken. I built it first and backed it out. There are already two mono stacks in the tree, Bootstrap's$font-family-monospaceand a hardcoded copy atstyles.scss:74, and they differ overSFMono-Regular. A token copied from either would have been a third. Worth settling separately by pointing$font-family-monospaceat one agreed stack, which is also the right moment to close the gap wheregenerate-tokens.mjsemits utilities for colour,radiusandshadowbut notfont-weight,durationoreasing.The rollout summary legend.
RolloutSummary.tsx:69renders a variant key too, fromrollout.ts:93. It is not a JSX swap like the other four:RolloutSummaryRowis{ label, percentage }with no way to mark a label as a key, the legend mixes keys with a proseControlentry, and the label doubles as the React key and asbarSegments[].key. Covering it means changing that type and threading it throughgetTrafficSegments, which is shared chart plumbing and reviews differently to this.Copyable. It adds a new interaction in four places, with an icon, a tooltip and a copied state, and @khvn26 parked it as not important for the immediate fix. Cheaper to add to this one component later than to four call sites now.
How did you test this code?
Manually, across all four surfaces: the Edit User Feature modal for a multivariate flag, the feature editor's variant labels, and the two experiments views. Variants with no
keyset still fall back toVariant_n.npx tsc --noEmitreports only errors already onmain.eslint --fixreports nothing.