refactor(ui/alerts): address code-review findings on alerting-enums
Follow-up to83837adaaddressing the critical-review feedback: - Duplicate ConditionKind type consolidated: the one in api/queries/alertRules.ts (which was nullable — wrong) is gone; single source of truth lives in this module. - Module moved out of api/ into pages/Alerts/ where it belongs. api/ is the data layer; labels + hide lists are view-layer concerns. - Hidden values formalised: Comparator.EQ and JvmAggregation.LATEST are intentionally not surfaced in dropdowns (noisy / wrong feature boundary, see in-file comments). They remain in the type unions so rules that carry those values save/load correctly — we just don't advertise them in the UI. - JvmAggregation declaration order restored to MAX/AVG/MIN (matches what users saw before83837ada). LATEST declared last; hidden. - Snapshot tests for every visible *_OPTIONS array — reviewer signal in future PRs when a backend enum change or hide-list edit silently reshapes the dropdown. - `toOptions` gains a JSDoc noting that label-map declaration order is load-bearing (ES2015 Object.keys insertion-order guarantee). - **Honest about the springdoc schema quirk**: the generated polymorphic condition types resolve to `never` at the TypeScript level (two conflicting `kind` discriminators — the class-name literal and the Jackson enum — intersect to never), which silently defeated `Record<T, string>` exhaustiveness. The previous commit's "schema-derived enums" claim was accurate only for the flat-field enums (ConditionKind, Severity, TargetKind); condition-specific enums (RouteMetric, Comparator, JvmAggregation, ExchangeFireMode) were silently `never`. Those are now declared as hand-written string-literal unions with a top-of-file comment spelling out the issue and the regen-and-compare workflow. Real upstream fix is a backend-side adjustment to how springdoc emits polymorphic `@JsonSubTypes` — out of scope for this phase. Verified: ui build green, 56/56 vitest pass (49 pre-existing + 7 new enum snapshots). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -6,7 +6,7 @@ import { AgentStateForm } from './condition-forms/AgentStateForm';
|
||||
import { DeploymentStateForm } from './condition-forms/DeploymentStateForm';
|
||||
import { LogPatternForm } from './condition-forms/LogPatternForm';
|
||||
import { JvmMetricForm } from './condition-forms/JvmMetricForm';
|
||||
import { CONDITION_KIND_OPTIONS } from '../../../api/alerting-enums';
|
||||
import { CONDITION_KIND_OPTIONS } from '../enums';
|
||||
|
||||
export function ConditionStep({ form, setForm }: { form: FormState; setForm: (f: FormState) => void }) {
|
||||
const onKindChange = (v: string) => {
|
||||
|
||||
@@ -3,7 +3,7 @@ import { useCatalog } from '../../../api/queries/catalog';
|
||||
import { useAgents } from '../../../api/queries/agents';
|
||||
import { useSelectedEnv } from '../../../api/queries/alertMeta';
|
||||
import type { FormState } from './form-state';
|
||||
import { SEVERITY_OPTIONS } from '../../../api/alerting-enums';
|
||||
import { SEVERITY_OPTIONS } from '../enums';
|
||||
|
||||
const SCOPE_OPTIONS = [
|
||||
{ value: 'env', label: 'Environment-wide' },
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { FormField, Input, Select } from '@cameleer/design-system';
|
||||
import type { FormState } from '../form-state';
|
||||
import { EXCHANGE_FIRE_MODE_OPTIONS } from '../../../../api/alerting-enums';
|
||||
import { EXCHANGE_FIRE_MODE_OPTIONS } from '../../enums';
|
||||
|
||||
// ExchangeFilter.status is typed as `String` on the backend (no @Schema
|
||||
// allowableValues yet) so options stay hand-typed. Follow-up: annotate the
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { FormField, Input, Select } from '@cameleer/design-system';
|
||||
import type { FormState } from '../form-state';
|
||||
import { JVM_AGGREGATION_OPTIONS, COMPARATOR_OPTIONS } from '../../../../api/alerting-enums';
|
||||
import { JVM_AGGREGATION_OPTIONS, COMPARATOR_OPTIONS } from '../../enums';
|
||||
|
||||
export function JvmMetricForm({ form, setForm }: { form: FormState; setForm: (f: FormState) => void }) {
|
||||
const c = form.condition as Record<string, unknown>;
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { FormField, Input, Select } from '@cameleer/design-system';
|
||||
import type { FormState } from '../form-state';
|
||||
import { ROUTE_METRIC_OPTIONS, COMPARATOR_OPTIONS } from '../../../../api/alerting-enums';
|
||||
import { ROUTE_METRIC_OPTIONS, COMPARATOR_OPTIONS } from '../../enums';
|
||||
|
||||
export function RouteMetricForm({ form, setForm }: { form: FormState; setForm: (f: FormState) => void }) {
|
||||
const c = form.condition as Record<string, unknown>;
|
||||
|
||||
@@ -1,10 +1,9 @@
|
||||
import type {
|
||||
AlertRuleRequest,
|
||||
AlertRuleResponse,
|
||||
ConditionKind,
|
||||
AlertCondition,
|
||||
} from '../../../api/queries/alertRules';
|
||||
import type { Severity, TargetKind } from '../../../api/alerting-enums';
|
||||
import type { ConditionKind, Severity, TargetKind } from '../enums';
|
||||
|
||||
export type WizardStep = 'scope' | 'condition' | 'trigger' | 'notify' | 'review';
|
||||
export const WIZARD_STEPS: WizardStep[] = ['scope', 'condition', 'trigger', 'notify', 'review'];
|
||||
|
||||
85
ui/src/pages/Alerts/enums.test.ts
Normal file
85
ui/src/pages/Alerts/enums.test.ts
Normal file
@@ -0,0 +1,85 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import {
|
||||
CONDITION_KIND_OPTIONS,
|
||||
SEVERITY_OPTIONS,
|
||||
ROUTE_METRIC_OPTIONS,
|
||||
COMPARATOR_OPTIONS,
|
||||
JVM_AGGREGATION_OPTIONS,
|
||||
EXCHANGE_FIRE_MODE_OPTIONS,
|
||||
TARGET_KIND_OPTIONS,
|
||||
} from './enums';
|
||||
|
||||
/**
|
||||
* Snapshot tests for the schema-derived option arrays.
|
||||
*
|
||||
* A backend enum change (or a hide-list change) will flip one of these
|
||||
* snapshots. Reviewing the diff in a PR is the intended signal to confirm
|
||||
* the change is intentional. The `Record<T, string>` label maps already
|
||||
* enforce exhaustiveness at compile time; these tests enforce the *visible*
|
||||
* shape (values, order, hidden items) at review time.
|
||||
*/
|
||||
|
||||
describe('alerts/enums option arrays', () => {
|
||||
it('CONDITION_KIND_OPTIONS', () => {
|
||||
expect(CONDITION_KIND_OPTIONS).toEqual([
|
||||
{ value: 'ROUTE_METRIC', label: 'Route metric (error rate, latency, throughput)' },
|
||||
{ value: 'EXCHANGE_MATCH', label: 'Exchange match (specific failures)' },
|
||||
{ value: 'AGENT_STATE', label: 'Agent state (DEAD / STALE)' },
|
||||
{ value: 'DEPLOYMENT_STATE', label: 'Deployment state (FAILED / DEGRADED)' },
|
||||
{ value: 'LOG_PATTERN', label: 'Log pattern (count of matching logs)' },
|
||||
{ value: 'JVM_METRIC', label: 'JVM metric (heap, GC, inflight)' },
|
||||
]);
|
||||
});
|
||||
|
||||
it('SEVERITY_OPTIONS', () => {
|
||||
expect(SEVERITY_OPTIONS).toEqual([
|
||||
{ value: 'CRITICAL', label: 'Critical' },
|
||||
{ value: 'WARNING', label: 'Warning' },
|
||||
{ value: 'INFO', label: 'Info' },
|
||||
]);
|
||||
});
|
||||
|
||||
it('ROUTE_METRIC_OPTIONS', () => {
|
||||
expect(ROUTE_METRIC_OPTIONS).toEqual([
|
||||
{ value: 'ERROR_RATE', label: 'Error rate' },
|
||||
{ value: 'P99_LATENCY_MS', label: 'P99 latency (ms)' },
|
||||
{ value: 'AVG_DURATION_MS', label: 'Avg duration (ms)' },
|
||||
{ value: 'THROUGHPUT', label: 'Throughput (msg/s)' },
|
||||
{ value: 'ERROR_COUNT', label: 'Error count' },
|
||||
]);
|
||||
});
|
||||
|
||||
it('COMPARATOR_OPTIONS (EQ is hidden — noisy for float comparisons)', () => {
|
||||
expect(COMPARATOR_OPTIONS).toEqual([
|
||||
{ value: 'GT', label: '>' },
|
||||
{ value: 'GTE', label: '\u2265' },
|
||||
{ value: 'LT', label: '<' },
|
||||
{ value: 'LTE', label: '\u2264' },
|
||||
]);
|
||||
expect(COMPARATOR_OPTIONS.find((o) => o.value === 'EQ')).toBeUndefined();
|
||||
});
|
||||
|
||||
it('JVM_AGGREGATION_OPTIONS (LATEST is hidden — use a dashboard instead)', () => {
|
||||
expect(JVM_AGGREGATION_OPTIONS).toEqual([
|
||||
{ value: 'MAX', label: 'MAX' },
|
||||
{ value: 'AVG', label: 'AVG' },
|
||||
{ value: 'MIN', label: 'MIN' },
|
||||
]);
|
||||
expect(JVM_AGGREGATION_OPTIONS.find((o) => o.value === 'LATEST')).toBeUndefined();
|
||||
});
|
||||
|
||||
it('EXCHANGE_FIRE_MODE_OPTIONS', () => {
|
||||
expect(EXCHANGE_FIRE_MODE_OPTIONS).toEqual([
|
||||
{ value: 'PER_EXCHANGE', label: 'One alert per matching exchange' },
|
||||
{ value: 'COUNT_IN_WINDOW', label: 'Threshold: N matches in window' },
|
||||
]);
|
||||
});
|
||||
|
||||
it('TARGET_KIND_OPTIONS', () => {
|
||||
expect(TARGET_KIND_OPTIONS).toEqual([
|
||||
{ value: 'USER', label: 'User' },
|
||||
{ value: 'GROUP', label: 'Group' },
|
||||
{ value: 'ROLE', label: 'Role' },
|
||||
]);
|
||||
});
|
||||
});
|
||||
149
ui/src/pages/Alerts/enums.ts
Normal file
149
ui/src/pages/Alerts/enums.ts
Normal file
@@ -0,0 +1,149 @@
|
||||
/**
|
||||
* Alerting option lists and enum types used by the rule editor.
|
||||
*
|
||||
* These are **string-literal unions mirrored by hand from the backend Java
|
||||
* enums**. Why not derived from `schema.d.ts`? Springdoc emits polymorphic
|
||||
* `@JsonSubTypes` conditions with two conflicting `kind` discriminators (the
|
||||
* class-name literal + the Jackson enum), whose intersection is `never` —
|
||||
* indexed access like `RouteMetricCondition['comparator']` resolves to
|
||||
* `never` and silently breaks `Record<T, string>` exhaustiveness. Until the
|
||||
* OpenAPI shape is fixed upstream, we declare the unions here.
|
||||
*
|
||||
* How to keep this file honest:
|
||||
* 1. Regenerate the schema (`npm run generate-api:live` or `generate-api`).
|
||||
* 2. Search `src/api/schema.d.ts` for the enum unions on the relevant
|
||||
* condition type (e.g. `comparator?: "GT" | "GTE" | …`).
|
||||
* 3. Update the matching union below.
|
||||
* 4. `enums.test.ts` dumps every exported `*_OPTIONS` array as an inline
|
||||
* snapshot — the diff in a PR shows exactly which values / labels moved.
|
||||
*
|
||||
* Fields whose backend type is `String` (agent state, log level, deployment
|
||||
* states, exchange filter status, JVM metric names) aren't in here at all —
|
||||
* springdoc emits them as open-ended strings. Follow-up: add
|
||||
* `@Schema(allowableValues = …)` on the Java record components, then migrate
|
||||
* those string literals into this file too.
|
||||
*
|
||||
* Condition-kind-wide enums (ConditionKind, Severity, TargetKind) are
|
||||
* derived from `schema.d.ts`, because they appear on flat fields that
|
||||
* springdoc types correctly.
|
||||
*/
|
||||
import type { components } from '../../api/schema';
|
||||
|
||||
type AlertRuleRequest = components['schemas']['AlertRuleRequest'];
|
||||
type AlertRuleTarget = components['schemas']['AlertRuleTarget'];
|
||||
|
||||
// Derived — these schema fields are correctly typed (flat, no polymorphism).
|
||||
export type ConditionKind = NonNullable<AlertRuleRequest['conditionKind']>;
|
||||
export type Severity = NonNullable<AlertRuleRequest['severity']>;
|
||||
export type TargetKind = NonNullable<AlertRuleTarget['kind']>;
|
||||
|
||||
// Manual — schema's polymorphic condition types resolve to `never`.
|
||||
// Mirrors: cameleer-server-core RouteMetric, Comparator, AggregationOp enums
|
||||
// and ExchangeMatchCondition.fireMode.
|
||||
export type RouteMetric = 'ERROR_RATE' | 'AVG_DURATION_MS' | 'P99_LATENCY_MS' | 'THROUGHPUT' | 'ERROR_COUNT';
|
||||
export type Comparator = 'GT' | 'GTE' | 'LT' | 'LTE' | 'EQ';
|
||||
export type JvmAggregation = 'MAX' | 'MIN' | 'AVG' | 'LATEST';
|
||||
export type ExchangeFireMode = 'PER_EXCHANGE' | 'COUNT_IN_WINDOW';
|
||||
|
||||
export interface Option<T extends string> { value: T; label: string }
|
||||
|
||||
/**
|
||||
* Build a dropdown option array from a `Record<T, string>` label map.
|
||||
*
|
||||
* Declaration order is load-bearing: ES2015+ guarantees `Object.keys` yields
|
||||
* string-keyed entries in insertion order, which is what the user sees.
|
||||
*
|
||||
* Hidden values stay in the `Record<T, string>` map (so `Record`-exhaustiveness
|
||||
* still pins labels to the union) and in the type `T` (so rules carrying a
|
||||
* hidden value round-trip fine through save/load) — they're simply filtered
|
||||
* out of the visible option array.
|
||||
*/
|
||||
function toOptions<T extends string>(labels: Record<T, string>, hidden?: readonly T[]): Option<T>[] {
|
||||
const skip: ReadonlySet<T> = new Set(hidden ?? []);
|
||||
return (Object.keys(labels) as T[])
|
||||
.filter((value) => !skip.has(value))
|
||||
.map((value) => ({ value, label: labels[value] }));
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Label maps — declaration order = dropdown order.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const CONDITION_KIND_LABELS: Record<ConditionKind, string> = {
|
||||
ROUTE_METRIC: 'Route metric (error rate, latency, throughput)',
|
||||
EXCHANGE_MATCH: 'Exchange match (specific failures)',
|
||||
AGENT_STATE: 'Agent state (DEAD / STALE)',
|
||||
DEPLOYMENT_STATE: 'Deployment state (FAILED / DEGRADED)',
|
||||
LOG_PATTERN: 'Log pattern (count of matching logs)',
|
||||
JVM_METRIC: 'JVM metric (heap, GC, inflight)',
|
||||
};
|
||||
|
||||
const SEVERITY_LABELS: Record<Severity, string> = {
|
||||
CRITICAL: 'Critical',
|
||||
WARNING: 'Warning',
|
||||
INFO: 'Info',
|
||||
};
|
||||
|
||||
const ROUTE_METRIC_LABELS: Record<RouteMetric, string> = {
|
||||
ERROR_RATE: 'Error rate',
|
||||
P99_LATENCY_MS: 'P99 latency (ms)',
|
||||
AVG_DURATION_MS: 'Avg duration (ms)',
|
||||
THROUGHPUT: 'Throughput (msg/s)',
|
||||
ERROR_COUNT: 'Error count',
|
||||
};
|
||||
|
||||
const COMPARATOR_LABELS: Record<Comparator, string> = {
|
||||
GT: '>',
|
||||
GTE: '\u2265',
|
||||
LT: '<',
|
||||
LTE: '\u2264',
|
||||
EQ: '=',
|
||||
};
|
||||
|
||||
// Previous UI ordering (MAX, AVG, MIN) preserved; LATEST is declared last
|
||||
// and hidden — see comment on JVM_AGGREGATION_HIDDEN below.
|
||||
const JVM_AGGREGATION_LABELS: Record<JvmAggregation, string> = {
|
||||
MAX: 'MAX',
|
||||
AVG: 'AVG',
|
||||
MIN: 'MIN',
|
||||
LATEST: 'LATEST',
|
||||
};
|
||||
|
||||
const EXCHANGE_FIRE_MODE_LABELS: Record<ExchangeFireMode, string> = {
|
||||
PER_EXCHANGE: 'One alert per matching exchange',
|
||||
COUNT_IN_WINDOW: 'Threshold: N matches in window',
|
||||
};
|
||||
|
||||
const TARGET_KIND_LABELS: Record<TargetKind, string> = {
|
||||
USER: 'User',
|
||||
GROUP: 'Group',
|
||||
ROLE: 'Role',
|
||||
};
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Hidden values — legal on the wire (saved/loaded rules round-trip fine) but
|
||||
// intentionally not surfaced in dropdowns. Document *why* — silent omission
|
||||
// is exactly what caused the original drift problem.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const COMPARATOR_HIDDEN: readonly Comparator[] = [
|
||||
'EQ', // Exact equality on floating-point metrics is rarely useful and noisy
|
||||
// — users should pick GT/LT with a sensible threshold instead.
|
||||
];
|
||||
|
||||
const JVM_AGGREGATION_HIDDEN: readonly JvmAggregation[] = [
|
||||
'LATEST', // Point-in-time reads belong on a metric dashboard, not an alert
|
||||
// rule — a windowed MAX/MIN/AVG is what you want for alerting.
|
||||
];
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Exported option arrays (visible in dropdowns).
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
export const CONDITION_KIND_OPTIONS: Option<ConditionKind>[] = toOptions(CONDITION_KIND_LABELS);
|
||||
export const SEVERITY_OPTIONS: Option<Severity>[] = toOptions(SEVERITY_LABELS);
|
||||
export const ROUTE_METRIC_OPTIONS: Option<RouteMetric>[] = toOptions(ROUTE_METRIC_LABELS);
|
||||
export const COMPARATOR_OPTIONS: Option<Comparator>[] = toOptions(COMPARATOR_LABELS, COMPARATOR_HIDDEN);
|
||||
export const JVM_AGGREGATION_OPTIONS: Option<JvmAggregation>[] = toOptions(JVM_AGGREGATION_LABELS, JVM_AGGREGATION_HIDDEN);
|
||||
export const EXCHANGE_FIRE_MODE_OPTIONS: Option<ExchangeFireMode>[] = toOptions(EXCHANGE_FIRE_MODE_LABELS);
|
||||
export const TARGET_KIND_OPTIONS: Option<TargetKind>[] = toOptions(TARGET_KIND_LABELS);
|
||||
Reference in New Issue
Block a user