Skip to content

Commit f23f1d6

Browse files
committed
fix(FR-2494): address code review findings across auto-scaling rule stack
1 parent dc12e0c commit f23f1d6

5 files changed

Lines changed: 51 additions & 29 deletions

File tree

react/src/components/AutoScalingRuleEditorModal.tsx

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,14 @@ const AutoScalingRuleEditorModal: React.FC<AutoScalingRuleEditorModalProps> = ({
309309
`);
310310

311311
const handleOk = () => {
312+
// Manual validation for Prometheus preset (Form.Item has no name, so
313+
// Ant Design form validation does not cover it automatically)
314+
const currentMetricSource = formRef.current?.getFieldValue('metricSource');
315+
if (currentMetricSource === 'PROMETHEUS' && !selectedPresetId) {
316+
message.error(t('autoScalingRule.PrometheusPresetRequired'));
317+
return;
318+
}
319+
312320
return formRef.current
313321
?.validateFields()
314322
.then((values) => {
@@ -512,12 +520,18 @@ const AutoScalingRuleEditorModal: React.FC<AutoScalingRuleEditorModalProps> = ({
512520
}
513521
}}
514522
options={[
515-
{ label: 'Kernel', value: 'KERNEL' },
516523
{
517-
label: 'Inference Framework',
524+
label: t('autoScalingRule.MetricSourceKernel'),
525+
value: 'KERNEL',
526+
},
527+
{
528+
label: t('autoScalingRule.MetricSourceInferenceFramework'),
518529
value: 'INFERENCE_FRAMEWORK',
519530
},
520-
{ label: 'Prometheus', value: 'PROMETHEUS' },
531+
{
532+
label: t('autoScalingRule.MetricSourcePrometheus'),
533+
value: 'PROMETHEUS',
534+
},
521535
]}
522536
/>
523537
</Form.Item>
@@ -606,19 +620,17 @@ const AutoScalingRuleEditorModal: React.FC<AutoScalingRuleEditorModalProps> = ({
606620
<Form.Item
607621
label={t('autoScalingRule.QueryTemplate')}
608622
extra={
609-
selectedPresetId ? (
610-
<ErrorBoundaryWithNullFallback>
611-
<React.Suspense
612-
fallback={
613-
<Spin size="small" style={{ marginRight: 8 }} />
614-
}
615-
>
616-
<PrometheusPresetPreview
617-
presetGlobalId={selectedPresetId}
618-
/>
619-
</React.Suspense>
620-
</ErrorBoundaryWithNullFallback>
621-
) : undefined
623+
<ErrorBoundaryWithNullFallback>
624+
<React.Suspense
625+
fallback={
626+
<Spin size="small" style={{ marginRight: 8 }} />
627+
}
628+
>
629+
<PrometheusPresetPreview
630+
presetGlobalId={selectedPreset.id}
631+
/>
632+
</React.Suspense>
633+
</ErrorBoundaryWithNullFallback>
622634
}
623635
>
624636
<Typography.Text

react/src/components/AutoScalingRuleEditorModalLegacy.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ type AutoScalingRuleInput = {
5050
max_replicas: number;
5151
};
5252

53-
export const COMPARATOR_LABELS: Record<string, string> = {
53+
export const COMPARATOR_LABELS: Record<'LESS_THAN' | 'GREATER_THAN', string> = {
5454
LESS_THAN: '<',
5555
GREATER_THAN: '>',
5656
};

react/src/components/AutoScalingRuleList.tsx

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ const AutoScalingRuleList: React.FC<AutoScalingRuleListProps> = ({
207207
<Button
208208
type="primary"
209209
icon={<PlusOutlined />}
210-
disabled={isEndpointDestroying}
210+
disabled={isEndpointDestroying || !isOwnedByCurrentUser}
211211
onClick={() => {
212212
setEditingRuleId(null);
213213
setIsOpenEditorModal(true);
@@ -225,9 +225,11 @@ const AutoScalingRuleList: React.FC<AutoScalingRuleListProps> = ({
225225
title: t('autoScalingRule.ScalingType'),
226226
fixed: 'left',
227227
render: (_text, row) =>
228-
(row?.stepSize || 0) > 0
229-
? t('autoScalingRule.ScaleOut')
230-
: t('autoScalingRule.ScaleIn'),
228+
row?.stepSize
229+
? row.stepSize > 0
230+
? t('autoScalingRule.ScaleOut')
231+
: t('autoScalingRule.ScaleIn')
232+
: '-',
231233
},
232234
{
233235
title: t('autoScalingRule.MetricSource'),
@@ -318,9 +320,10 @@ const AutoScalingRuleList: React.FC<AutoScalingRuleListProps> = ({
318320
</span>
319321
),
320322
sorter: (a, b) => {
321-
const date1 = dayjs(a.createdAt);
322-
const date2 = dayjs(b.createdAt);
323-
return date1.diff(date2);
323+
if (!a.lastTriggeredAt && !b.lastTriggeredAt) return 0;
324+
if (!a.lastTriggeredAt) return -1;
325+
if (!b.lastTriggeredAt) return 1;
326+
return dayjs(a.lastTriggeredAt).diff(dayjs(b.lastTriggeredAt));
324327
},
325328
},
326329
{
@@ -355,7 +358,11 @@ const AutoScalingRuleList: React.FC<AutoScalingRuleListProps> = ({
355358
okButtonProps={{
356359
danger: true,
357360
}}
358-
disabled={isInFlightDeleteMutation}
361+
disabled={
362+
isInFlightDeleteMutation ||
363+
isEndpointDestroying ||
364+
!isOwnedByCurrentUser
365+
}
359366
onConfirm={() => {
360367
if (row) {
361368
commitDeleteMutation({
@@ -435,7 +442,6 @@ const AutoScalingRuleList: React.FC<AutoScalingRuleListProps> = ({
435442
handleRefetch();
436443
}
437444
}}
438-
onComplete={handleRefetch}
439445
/>
440446
</>
441447
);

react/src/components/AutoScalingRuleListLegacy.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ const renderCondition = (row: any) => {
6565
<BAIFlex gap={'xs'}>
6666
{threshold}
6767
{suffix}
68-
{comparator ? <Tooltip title={comparator}>{'<'}</Tooltip> : '-'}
68+
<Tooltip title={comparator}>{'<'}</Tooltip>
6969
<Tag>{metricName}</Tag>
7070
</BAIFlex>
7171
);
@@ -77,7 +77,8 @@ const renderCondition = (row: any) => {
7777
<Tag>{metricName}</Tag>
7878
{comparator ? (
7979
<Tooltip title={comparator}>
80-
{COMPARATOR_LABELS[comparator] || comparator}
80+
{COMPARATOR_LABELS[comparator as keyof typeof COMPARATOR_LABELS] ||
81+
comparator}
8182
</Tooltip>
8283
) : (
8384
'-'

resources/i18n/en.json

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,9 @@
167167
"MaxThresholdRequired": "Max threshold is required.",
168168
"MetricName": "Metric Name",
169169
"MetricSource": "Metric Source",
170+
"MetricSourceInferenceFramework": "Inference Framework",
171+
"MetricSourceKernel": "Kernel",
172+
"MetricSourcePrometheus": "Prometheus",
170173
"MinMustBeLessThanMax": "Min threshold must be less than max threshold.",
171174
"MinReplicas": "Min Replicas",
172175
"MinThreshold": "Min Threshold",
@@ -185,7 +188,7 @@
185188
"Single": "Single",
186189
"StepSize": "Step Size",
187190
"SuccessfullyCreated": "Auto scaling rule has been successfully created.",
188-
"SuccessfullyDeleted": "Auto scaling rule has been successfuly deleted.",
191+
"SuccessfullyDeleted": "Auto scaling rule has been successfully deleted.",
189192
"SuccessfullyUpdated": "Auto scaling rule has been successfully updated.",
190193
"Threshold": "Threshold",
191194
"ThresholdMustBePositive": "Threshold must be a positive number.",

0 commit comments

Comments
 (0)