feat: label variant key UI#7745
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Failed testsfirefox › tests/segment-test.pw.ts › Segment test 1 - Create, update, and manage segments with multivariate flags @oss Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Failed testsfirefox › tests/change-request-test.pw.ts › Change Request Tests › Change requests can be created, approved, and published with 4-eyes approval @enterprise Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Failed testsfirefox › tests/change-request-test.pw.ts › Change Request Tests › Change requests can be created, approved, and published with 4-eyes approval @enterprise Details
|
Visual Regression15 screenshots compared. See report for details. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to define and edit stable, human-readable keys (labels) for multivariate feature variations in the UI. It refactors the feature modal's variation tab to display variants as cards, adds inline editing for variant labels, and tracks unsaved changes with a 'Not saved' badge. Additionally, it updates the Input and ContentCard components, adds corresponding Storybook stories, and includes Playwright E2E tests. The review feedback highlights three key improvements: ensuring unique default variant names by accounting for fallback display names, normalizing null and undefined values to prevent false positives on the 'Not saved' badge, and falling back to an empty string for cleared weight inputs to avoid React controlled-to-uncontrolled warnings.
|
/gemini review |
|
@gemini can you please have a thorough look of the data flow from the creation of a multivariate option, it's initial creation (POST) request and a direct update (PUT) without any full refresh |
|
The data flow for multivariate options in this PR follows a batch-save pattern, where changes are managed in the local state of the 1. State Initialization
2. Local Updates
3. Change Tracking
4. Persistence (Save Flow)
Considerations for "Direct Updates"If your requirement is to perform a direct update (immediate PUT) on every change without waiting for the modal save, you would need to:
Currently, the batch-save approach is safer for maintaining consistency across the multivariate options, especially since the |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to set and edit stable, human-readable keys (labels) for multivariate feature variations in the frontend. It adds inline editing with validation for variant keys, tracks unsaved variations, updates UI components like Input and ContentCard to support these changes, and updates E2E tests and Storybook stories. A review comment correctly identified a bug in mergeEnvironmentWeights where using the logical OR operator (||) instead of nullish coalescing (??) would cause an environment weight of 0 to incorrectly fall back to the default percentage allocation.
| const mergeEnvironmentWeights = (options: any[], variations: any[]): any[] => | ||
| options.map((v: any) => { | ||
| const matchingVariation = variations.find( | ||
| (e: any) => e.multivariate_feature_option === v.id, | ||
| ) | ||
| return { | ||
| ...v, | ||
| default_percentage_allocation: | ||
| (matchingVariation && matchingVariation.percentage_allocation) || | ||
| v.default_percentage_allocation || | ||
| 0, | ||
| } | ||
| }) |
There was a problem hiding this comment.
In mergeEnvironmentWeights, using the logical OR operator (||) to fall back to v.default_percentage_allocation when matchingVariation.percentage_allocation is falsy introduces a bug. If the environment weight is explicitly set to 0 (which is falsy in JavaScript), it will incorrectly fall back to the default percentage allocation instead of preserving the 0 value.
Using nullish coalescing (??) resolves this issue by only falling back when the value is null or undefined.
| const mergeEnvironmentWeights = (options: any[], variations: any[]): any[] => | |
| options.map((v: any) => { | |
| const matchingVariation = variations.find( | |
| (e: any) => e.multivariate_feature_option === v.id, | |
| ) | |
| return { | |
| ...v, | |
| default_percentage_allocation: | |
| (matchingVariation && matchingVariation.percentage_allocation) || | |
| v.default_percentage_allocation || | |
| 0, | |
| } | |
| }) | |
| const mergeEnvironmentWeights = (options: any[], variations: any[]): any[] => | |
| options.map((v: any) => { | |
| const matchingVariation = variations.find( | |
| (e: any) => e.multivariate_feature_option === v.id, | |
| ) | |
| return { | |
| ...v, | |
| default_percentage_allocation: | |
| matchingVariation?.percentage_allocation ?? | |
| v.default_percentage_allocation ?? | |
| 0, | |
| } | |
| }) |
talissoncosta
left a comment
There was a problem hiding this comment.
Looks pretty @Zaimwa9 .
Thanks for addressing the new design and writing the stories.
I caught this on storybook:
- The input underlined variant does not work well on dark mode yet. Do you mind double check that ?
Overall looks great!
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Adds UI to set a label on multivariate variants (maps to the backend
keyfield) and revamps the variants section of the feature modal's Value tab.Variant labels
controlreserved, unique per feature).Variant_n, first free number) that is persisted on save; existing variants with no key fall back to a display-only placeholder./mv-options/endpoint.Improvements
Variants UI revamp
experimental_flags).Inputgains reusableunderlineandcenteredprops; aligned the value editor height with adjacent inputs.Experiment setup page
ContentCardgains adescriptionprop used by the setup step.How did you test this code?
editVariantLabeledits a variant's label via the new UI, saves, reopens the modal, and asserts persistence. Audited existing selectors (add-variation,featureVariationValue*,featureVariationWeight*,delete-multivariate) for compatibility with the new layout.Inputvariants (Underline,UnderlineCentered).Variant_1,Variant_2…control, duplicates rejected with inline errors) and the "Not saved" chip appearing only on edited variants.nulland the placeholder returns.