Implement Upsert Metric Modal for Experiments v2

[zackcl]

Resolves #2631

@danoswaltCL I've opened this new PR since the old PR #2648 had too many conflicts with the latest experiment-design-refresh branch. I've applied all the changes in #2648 in this PR and then made these further changes:

• Filtered out all reward metric options in the Metric ID field. (Needs confirmation - let me know if the reward metric key of the current experiment should remain visible.) (fixes the comment)
• Disallowed editing and deleting reward metrics.
• Added validation requiring metric class, key, and ID selection from the provided options, with cascading field clearing. (fixes the comment)
• Prevented the Save button from being incorrectly enabled in edit mode for categorical metrics. (fixes the comment)
• Applied the appTrimInput directive to all input fields in UpsertMetricModalComponent.

Here are some of your comments that are unresolved and needs discussion:

• Need extra top/bottom paddings for the inner scrollable content in the modal (comment) Dan: disregard
• The "Description" field is missing from the submitted request. Do we even store this in the DB? If not, should we remove this field from the modal? (comment) Dan: this can be removed
• Maybe this modal needs a speedbump (comment) Dan: disregard, not needed!
• Maybe we should improve the display of the Metrics table ([comment] (Implement Upsert Metric Modal for Experiments #2648 (comment))) Dan: this should be a new story for later, @zackcl maybe some draft design ticket? Zack: I created a placeholder ticket: Improve the display of the Metrics table #2774

[Copilot]

Pull request overview

This PR implements a comprehensive Upsert Metric Modal for Experiments v2, enabling users to add and edit metrics with proper validation and user experience improvements.

Key Changes:

• Introduced a new UpsertMetricModalComponent with support for both global and repeatable metrics, including cascading field validation and autocomplete functionality
• Added MetricHelperService to handle metric CRUD operations (add, update, delete) with proper state management integration
• Implemented proper form validation requiring metric selection from provided options with cascading field clearing when parent selections change

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 19 comments.

Show a summary per file

File	Description
types/src/index.ts	Exports new METRIC_TYPE enum for type safety across the application
types/src/Experiment/enums.ts	Defines METRIC_TYPE enum with GLOBAL and REPEATABLE values
frontend/.../i18n/en.json	Adds hint text translations for metric form fields
frontend/.../common-dialog.service.ts	Adds methods to open add/edit/delete metric modals with proper configuration
frontend/.../experiment-metrics-section-card.component.ts	Integrates metric modal dialogs and handles metric row actions (edit, delete)
frontend/.../experiment-metrics-section-card.component.html	Removes unused menu button and wires up add metric functionality
frontend/.../upsert-metric-modal.component.ts	Implements comprehensive modal logic with autocomplete, validation, and metric type detection
frontend/.../upsert-metric-modal.component.html	Provides responsive form template with conditional field visibility and validation messages
frontend/.../upsert-metric-modal.component.scss	Defines styling for disabled states and section spacing
frontend/.../experiments.reducer.ts	Adds reducer cases for metric update actions with loading states
frontend/.../experiments.model.ts	Defines interfaces for metric form data and update requests
frontend/.../experiments.effects.ts	Implements effect for updating experiment metrics with success/error notifications
frontend/.../experiments.actions.ts	Defines actions for metric update flow (request, success, failure)
frontend/.../metric-helper.service.ts	Provides service methods for add, update, and delete metric operations
frontend/.../experiments.service.ts	Exposes updateExperimentMetrics method to dispatch update action
frontend/.../experiments.data.service.ts	Implements API call for updating experiment metrics

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[danoswaltCL]

@zackcl functionally this appears to be working well, I saw no issues clicking around and trying to do dumb weird things.

I hadn't looked into the code closely myself before I pressed the copilot review button, I was mainly looking for UI issues first...maybe ignore those for a second...

In the upsert-metric-modal.component.ts, there's a lot of code/complexity and type ambiguity (any and loose types that represent multiple potential things)... but I think it mostly comes out of trying to use one form and one basic form type to serve two pretty different metric types, global and repeated, with different behaviors.

My gut says that the complexity would go way way down if these were treated as two different forms with their own types and behaviors and files, with shared helper functions. Or at least two different base Form types instead of one with lots of optional properties, for example.

I'm not officially asking for that specific refactor, this is working and you spent a lot of time on it, but if we don't do something like that, I think this at least needs stricter types and unit tests, along with tsdoc comments. Maybe some way of reducing the size via moving logic to services? a 1000 lines component is a lot. I wouldn't mind spending some time with Claude trying to see if the two-form approach would help though if I have some time.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[danoswaltCL]

@zackcl i've been playing with claude code and wanted to share my thoughts via these branches. I have not touched what it spit out so some parts may be silly, but i tested in the UI and things still worked the same as far as I can tell. There were 3 things I asked it to do, eliminate any an improve the core types, refactor into some services to reduce the size of the component, and to brainstorm if it would be worth it to break into two forms.

I think the main thing that we need regardless is the types, here's how claude went about that, I think the naming of things could be improved but this is the idea and it's pretty simple:

https://github.com/CarnegieLearningWeb/UpGrade/compare/feature/2631-upsert-metric-modal-v2...wip/improved-types-metric-modal?expand=1

Then I asked it to expand on that branch with the types fixed to reduce the size of the component, here's what it did to pull certain large chunks into feature and shared services:

https://github.com/CarnegieLearningWeb/UpGrade/compare/feature/2631-upsert-metric-modal-v2...wip/improved-types-metric-modal-refactored-into-services?expand=1

I also asked it to plan out what it would be like to split into two forms for the two types and give a pro/con list. I can share that with you, it would have been an easier to implement had it started this way because it really would reduce the complexity a lot (no need to always be checking types and transforming between things manually when toggled), but I guess the work to refactor is probably too much at this point, so nevermind on that.

METRIC_MODAL_REFACTORING_PLAN.md

Please consider the 2 branches shared, they are for reference, I haven't thoroughly code reviewed them but wanted to share the idea. I think at minimum strict types, unit-tests, and tsdoc comments at least some of the heavier functions are needed since this is a very big and very complicated component. Refactoring some of it into services would be nice also (services are also a bit easier to write unit-tests for also btw).

[zackcl]

@danoswaltCL Thanks for taking the time to explore those branches and for sharing the plan. Here’s where I ended up after going through them:

• I tightened the modal form typing on our branch so we now lean on the shared IMetricUnit/ExperimentQueryPayload shapes instead of any, and I added a focused spec (npm test -- upsert-metric-modal → 3/3 passing for add flow, edit initialization, and DTO build).
• Added short TSDoc notes on the main entry points (form setup, edit wiring, payload prep) to document how the two metric paths work today.

I reviewed your “improved-types” branch. The approach swaps in new MetricOption/MetricFormControlValue types, but that drops support for the array keys the backend still sends for repeatable metrics and it changes the display formatting we already fixed, so I’d rather stick with the shared IMetricUnit contract we use now. The “refactored-into-services” branch mainly wraps the same logic in new helpers but depends on those new types, so we’d hit the same compatibility issues.

On the longer term idea of splitting the modal into two dedicated forms: I agree that would simplify state management if we tackle it later (maybe as a follow-up story). For this PR, the stricter typing + tests + docs are in place, and everything still passes locally. Let me know if there’s anything else you’d like me to adjust right now; happy to keep iterating.

[danoswaltCL]

Thanks Zack, this does look better with the types and stuff thank you.

Later will probably never come, we rarely have time to really circle back to tech-debt things, which is why i feel compelled to be annoying about it, to get as much of that as we can in this PR now without spending too long on it. It's just a very large component. My biggest fear in Angular is reactive forms. They turn into a nightmare fast if they aren't kept as simple as possible, that's more of the actual issue I'm worried about than the raw number of lines. But it is organized and you don't have gigantic run-on-sentence functions. Good types should help a lot also.

Let's just chat in refinement and see what Ben thinks also.