feat(experiments): attach a primary metric in the create experiment wizard#7780
feat(experiments): attach a primary metric in the create experiment wizard#7780Zaimwa9 wants to merge 14 commits into
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 (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - 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
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-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
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new CenteredModal component, enhances SelectableCard and ContentCard with additional layout options, and integrates a measurement step into the experiment creation wizard to allow selecting a primary metric and its expected direction. It also optimizes warehouse connection queries by adding an option to exclude event stats and prevents selecting feature flags already associated with active experiments. The review feedback highlights critical issues, including a mismatch in cache keys during pessimistic updates, a loading state (isEnabling) that gets stuck on successful warehouse connection creation, a hardcoded page size limit of 100 when checking for active experiments, and a potential runtime crash due to a missing defensive check on experiment.feature.
| const { data: experimentsData } = useGetExperimentsQuery( | ||
| { environmentId, page: 1, page_size: 100 }, | ||
| { skip: !environmentId }, | ||
| ) |
There was a problem hiding this comment.
The query for experiments is hardcoded with page_size: 100. If an environment has more than 100 experiments, active experiments beyond the first page will be excluded from the results, allowing users to select a feature flag that is already associated with an active experiment. Consider fetching all active experiments or implementing this validation on the backend.
There was a problem hiding this comment.
true but theoritcal, for now deferred
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to select a primary metric and define its expected direction during the experiment creation wizard. It also adds a reusable CenteredModal component, updates UI cards (SelectableCard and ContentCard) to support tags and descriptions, and prevents users from selecting feature flags that are already associated with active experiments. The review feedback highlights three key issues: the need to validate the final Review step in the wizard to disable the launch button when previous steps are invalid, fixing a state desynchronization issue when clearing the expected direction dropdown, and increasing the hardcoded page size when querying existing experiments to avoid missing active flags.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Wires up the Measurement step of the Create Experiment wizard so a primary metric can be attached when creating an experiment.
How did you test this code?
Manually, against a local API + warehouse:
Plus
npm run typecheckandeslinton the touched files.