feat(experiments): frequentist UI metric duplication #33631
Conversation
There was a problem hiding this comment.
PR Summary
Refactored metric duplication in experiments to centralize logic within Kea state management, moving away from utility functions to action-based duplication.
- Moved
duplicateMetricfrom utility function inDeltaChart.tsxto action inexperimentLogic.tsxfor better state management - Added duplication support in new metrics UI through
MetricRow.tsxcomponent - Simplified duplication interface to only require
metricIndexandisSecondaryparameters - Implemented proper naming for duplicated metrics, appending '(copy)' while handling unnamed metrics correctly
This follows single responsibility principle by centralizing metric duplication logic.
3 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
| : originalMetric.kind === NodeKind.ExperimentMetric | ||
| ? `${getDefaultMetricTitle(originalMetric)} (copy)` |
There was a problem hiding this comment.
The NodeKind enum is referenced in the duplicateMetric reducer but is missing an import. It was previously imported in DeltaChart.tsx but needs to be added to experimentLogic.tsx now that the duplication logic has been moved. Please add:
import { NodeKind } from '~/queries/schema/schema-general'to the imports in experimentLogic.tsx to resolve this reference.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
Size Change: -2.54 kB (-0.14%) Total Size: 1.83 MB
|
Not opposed to the move. I'm just hesitant to add more code to a module with 2000+ lines of code. The We could explore options to compose logic from these domain-specific logics, and One thing I would remove from the global state is all the modal booleans. The boilerplate is just not justifiable for how cheap a |
There was a problem hiding this comment.
Looks good to me 👍
I don't have a strong opinion in this specific case, the function is only used here. But I do like that we move towards simpler logic files, and move out logic to pure functions where it makes sense. It does make the logic files easier to read and work with.
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Changes
Added metric duplication in the new metrics UI.
As part of this, I'm proposing a refactor of where the duplication method lives. I think this method belongs in the Kea logic, since it operates directly on the logic value. Keeping it centralized helps keep things clean (while I’m still open to splitting
experimentLogicinto smaller logics). Happy to hear your thoughts @rodrigoiHow did you test this code?
👀