add warning icon, some refactoring to improve shared logic between components / selectors

[danoswaltCL]

• Makes equal weight the default when adding in new experiment
• Creates a common warning icon with tooltip
• Adds standalone Warning Icon capability to common section card title header component
• Refactors common chip status to use the common warning icon within it
• Adds functionality to pop that warning up when custom weights are indicated weights don't equal 100 (within 0.1)
• Refactors to build on the condition-helper service to share common weighting utilities between components and selectors
• Utilized Claude to create a bunch of unit tests for the condition-helper service

[bcb37]

This looks comprehensive to me.
One issue: if you have, say, two conditions and you've given them custom weights, then you delete one of the conditions, the warning will show, but the weight will not be editable - because we disable weight editing if there are < 2 conditions.
I can't remember if we decided that was OK. It's awkward, but you'd have to add at least one more condition to make the experiment valid anyway.

[danoswaltCL]

🤔 true... where did this restriction on editing weights when only one condition come from, there's no reason i can see to do that? @zackcl @amurphy-cl can we briefly touch on this in the refinement.

[danoswaltCL]

@bcb37 I've updated this one so that the weights modal isn't disabled.

[Copilot]

Pull Request Overview

This PR adds warning icon functionality and refactors experiment condition weighting logic to improve code reusability. It introduces a new CommonWarningIconComponent, makes equal weighting the default for experiments, and consolidates weighting utilities into the ConditionHelperService with comprehensive unit tests.

Key Changes:

• Creates reusable CommonWarningIconComponent with tooltip support for displaying warnings across the application
• Refactors condition weighting logic into pure functions (determineWeightingMethod, distributeWeightsEqually, isWeightSumValid) for use in selectors and components
• Adds weight validation warnings that display when custom weights don't sum to 100% (within tolerance)

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
frontend/projects/upgrade/src/assets/i18n/en.json	Moves translation keys from feature-flag specific to common component namespace and adds new keys for timestamps
frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts	Updates openEditConditionWeightsModal to accept weightingMethod parameter
frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-warning-icon/*	Adds new reusable warning icon component with Material icon and tooltip
frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-status-indicator-chip/*	Refactors to use CommonWarningIconComponent and changes from boolean showWarning to string warningMessage
frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-section-card-title-header/*	Adds standalone warning icon support, removes versionNumber input, updates translation keys
frontend/projects/upgrade/src/app/features/dashboard/segments/pages/.../segment-overview-details-section-card.component.html	Removes obsolete versionNumber binding
frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/.../feature-flag-overview-details-section-card.component.html	Updates to use new warningMessage pattern with conditional translation
frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/.../experiment-conditions-table.component.html	Removes disabled state from edit weights button and applies formatting fixes
frontend/projects/upgrade/src/app/features/dashboard/experiments/pages/.../experiment-conditions-section-card.component.*	Integrates weight validation warning display using new selector
frontend/projects/upgrade/src/app/features/dashboard/experiments/modals/edit-condition-weights-modal/edit-condition-weights-modal.component.*	Refactors to use shared weighting utilities, fixes typo in method name, uses WEIGHTING_METHOD constants
frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts	Adds weightingMethod to selected experiment, creates new selectConditionWeightsValid selector
frontend/projects/upgrade/src/app/core/experiments/store/experiments.selector.spec.ts	Updates test expectations to include new stat and weightingMethod properties
frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts	Changes adapter type to ExperimentVM and adds properties to initial state
frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts	Adds WeightingMethod type and WEIGHTING_METHOD constants, updates ExperimentVM and ExperimentState interfaces
frontend/projects/upgrade/src/app/core/experiments/condition-helper.service.ts	Adds pure functions and WEIGHT_CONFIG for shared weighting logic, updates service methods to auto-distribute weights when appropriate
frontend/projects/upgrade/src/app/core/experiments/condition-helper.service.spec.ts	Adds comprehensive unit tests (468 lines) covering all new pure functions and service methods

---

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

[Copilot]

The VALIDATION_TOLERANCE is set to 0.1, which allows totals from 99.9% to 100.1%. However, the comment states this is to "Allow rounding errors from equal distribution (e.g., 3 × 33.33 = 99.99)".

This tolerance of 0.1 is actually too large. For equal distribution scenarios:

• 3 conditions: 3 × 33.33 = 99.99 (only 0.01 off)
• Most equal distributions will be within 0.01 of 100

A tolerance of 0.1 would also accept clearly incorrect totals like 90.1% or 109.9% for custom weights. Consider reducing this to 0.01 or 0.05 to better match actual rounding errors while catching genuine validation issues.

Suggested change

	VALIDATION_TOLERANCE: 0.1, // Allow rounding errors from equal distribution (e.g., 3 × 33.33 = 99.99)
	VALIDATION_TOLERANCE: 0.01, // Allow rounding errors from equal distribution (e.g., 3 × 33.33 = 99.99)

[Copilot]

The determineWeightingMethod function is missing a validation that was present in the old implementation (lines 127-159 in edit-condition-weights-modal.component.ts). The old code checked:

1. If weights are equally distributed
2. AND if total is close to 100%

The new implementation only checks if all weights match the expected equal weight, but doesn't verify the sum is close to 100%. This means conditions like [{weight: 10}, {weight: 10}, {weight: 10}] (total 30%) would incorrectly return 'equal' instead of 'custom'.

Add a validation to check that the total weight is close to WEIGHT_CONFIG.TOTAL_WEIGHT before returning WEIGHTING_METHOD.EQUAL:

const totalWeight = conditions.reduce((sum, c) => sum + (c.assignmentWeight || 0), 0);
const isTotalValid = Math.abs(totalWeight - WEIGHT_CONFIG.TOTAL_WEIGHT) < WEIGHT_CONFIG.VALIDATION_TOLERANCE;

return isEqual && isTotalValid ? WEIGHTING_METHOD.EQUAL : WEIGHTING_METHOD.CUSTOM;

Suggested change

	return isEqual ? WEIGHTING_METHOD.EQUAL : WEIGHTING_METHOD.CUSTOM;
	const totalWeight = conditions.reduce((sum, c) => sum + (c.assignmentWeight || 0), 0);
	const isTotalValid = Math.abs(totalWeight - WEIGHT_CONFIG.TOTAL_WEIGHT) < WEIGHT_CONFIG.VALIDATION_TOLERANCE;
	return isEqual && isTotalValid ? WEIGHTING_METHOD.EQUAL : WEIGHTING_METHOD.CUSTOM;

[Copilot]

The test expects the total to be exactly 99.99, but the comment on line 153 says "Total should be 99.99 (acceptable per requirements)". However, using .toBe(99.99) for floating point comparison can be fragile due to precision issues.

Consider using .toBeCloseTo(99.99, 2) instead of .toBe(99.99) for more reliable floating point comparison. This is especially important since line 155 already demonstrates the total calculation, and floating point arithmetic can introduce small precision errors.

Suggested change

	expect(total).toBe(99.99);
	expect(total).toBeCloseTo(99.99, 2);

[Copilot]

[nitpick] The ternary expression passes an empty string when shouldShowWarning$ is false: (shouldShowWarning$ | async) ? ('feature-flags.global-status-warning-tooltip.text' | translate) : ''

However, this could be simplified since the CommonStatusIndicatorChipComponent already handles the absence of a warning message with *ngIf="warningMessage". Instead of passing an empty string, you could pass undefined or use a conditional approach:

[warningMessage]="(shouldShowWarning$ | async) ? ('feature-flags.global-status-warning-tooltip.text' | translate) : undefined"

Or better yet, create a computed observable in the component that returns the translated string or undefined directly, keeping the template cleaner.

Suggested change

	(shouldShowWarning$ | async) ? ('feature-flags.global-status-warning-tooltip.text' | translate) : ''
	(shouldShowWarning$ | async) ? ('feature-flags.global-status-warning-tooltip.text' | translate) : undefined

[Copilot]

The stat and weightingMethod properties are being added to the initialState object of the experiments reducer, but these properties belong to individual experiments (ExperimentVM), not to the ExperimentState itself. The ExperimentState uses an EntityAdapter which stores individual experiments in an entities object. These properties should not be at the root level of the state.

This will cause type errors and incorrect state structure. These properties should only exist on individual ExperimentVM objects within the entities collection, not on the ExperimentState root.

Suggested change

	stat: null,
	weightingMethod: 'equal',

[Copilot]

The selector has an explicit return type annotation (routerState, experimentState): ExperimentVM => but it can return undefined when there's no matching experiment. The return type should be ExperimentVM | undefined to accurately reflect this possibility.

This could lead to runtime errors if code assumes the selector always returns an ExperimentVM object.

Suggested change

	(routerState, experimentState): ExperimentVM => {
	(routerState, experimentState): ExperimentVM | undefined => {

[Copilot]

The button has its [disabled] attribute removed, but in the original code it was disabled when actionsDisabled is true or when there are fewer than 2 conditions ([disabled]="actionsDisabled || conditions.length < 2").

Removing the disabled state entirely means users can click the edit weights button even when there's only 1 condition (where weight editing doesn't make sense) or when actions should be disabled. This could lead to unexpected behavior or errors.

The [disabled] binding should be restored with appropriate logic.

Suggested change

	<button mat-icon-button class="weight-edit-button" (click)="onEditWeightsClick()">
	<button mat-icon-button class="weight-edit-button" [disabled]="actionsDisabled || conditions.length < 2" (click)="onEditWeightsClick()">

[Copilot]

The warningMessage is being translated twice - once in the parent component template and once in the child component. In the parent template, [warningMessage]="warningMessage | translate" applies the translate pipe, but the child CommonWarningIconComponent also likely expects a string that may already be translated.

This creates ambiguity about whether warningMessage should be a translation key or a translated string. If the child component template also uses | translate, this would attempt to translate an already-translated string.

Clarify the contract: either the parent or the child should handle translation, not both. Based on the warning icon component template (line 3 in common-warning-icon.component.html), it doesn't use the translate pipe, so the parent translation is correct, but this should be documented.

[Copilot]

The stat property is changed from required to optional (stat: IExperimentEnrollmentDetailStats → stat?: IExperimentEnrollmentDetailStats), but the selector returns stat: null for experiments without stats. This creates an inconsistency: null is not the same as undefined in TypeScript.

For better type safety and consistency, either:

1. Keep stat as required but always provide a value (including null)
2. Make it optional but return undefined instead of null when stats are not available

The current approach mixes both patterns.

[Copilot]

The comment states "All conditions get the exact same weight" but this is not accurate when the total doesn't divide evenly. For example, with 3 conditions, each gets 33.33% which totals 99.99%, not 100%. The comment immediately contradicts itself by noting "Total may be 99.99% or 100.01%".

Update the comment to accurately reflect that all conditions get the same rounded weight, but the total may not be exactly 100% due to rounding.

Suggested change

	* All conditions get the exact same weight rounded to 2 decimals.
	* Total may be 99.99% or 100.01% due to rounding - this is acceptable.
	* All conditions get the same weight, rounded to 2 decimals.
	* Due to rounding, the total may not be exactly 100% (e.g., 99.99% or 100.01%)—this is acceptable.

[bcb37]

This looks ok to me, besides the copilot suggestions.