Skip to content

Commit 3fea5cf

Browse files
claudialphonse78claudialphonse78
authored
Fix(RHOAIENG-57843): Added error indication on token limits when models are added (opendatahub-io#7247)
* fix: added error indication on token limits when models are added prior to editing enabling on edit * fix: removed unused handler and updated the removed indices logic
1 parent d9d9283 commit 3fea5cf

4 files changed

Lines changed: 113 additions & 86 deletions

File tree

‎packages/maas/frontend/src/app/hooks/__tests__/useSubscriptionModels.spec.ts‎

Lines changed: 73 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { act } from '@testing-library/react';
22
import { testHook } from '~/__tests__/unit/testUtils/hooks';
3-
import { useSubscriptionModels } from '~/app/hooks/useSubscriptionModels';
3+
import { reindexAfterRemove, useSubscriptionModels } from '~/app/hooks/useSubscriptionModels';
44
import { MaaSModelRefSummary, SubscriptionModelEntry } from '~/app/types/subscriptions';
55

66
const makeModelRef = (name: string, namespace = 'maas-models'): MaaSModelRefSummary => ({
@@ -18,6 +18,21 @@ const makeEntry = (
1818
tokenRateLimits,
1919
});
2020

21+
describe('reindexAfterRemove', () => {
22+
it('returns null when the edited row was removed', () => {
23+
expect(reindexAfterRemove(1, [1])).toBeNull();
24+
});
25+
26+
it('decrements the target when rows above it were removed', () => {
27+
expect(reindexAfterRemove(2, [0])).toBe(1);
28+
expect(reindexAfterRemove(3, [1, 2])).toBe(1);
29+
});
30+
31+
it('leaves the target unchanged when only rows below it were removed', () => {
32+
expect(reindexAfterRemove(0, [2])).toBe(0);
33+
});
34+
});
35+
2136
describe('useSubscriptionModels', () => {
2237
it('should initialize with empty models by default', () => {
2338
const renderResult = testHook(useSubscriptionModels)();
@@ -76,6 +91,55 @@ describe('useSubscriptionModels', () => {
7691
expect(renderResult.result.current.models[1].modelRefSummary.name).toBe('model-c');
7792
});
7893

94+
it('should reindex editLimitsTarget when a row above the target is removed', () => {
95+
const initial = [makeEntry('model-a'), makeEntry('model-b'), makeEntry('model-c')];
96+
const renderResult = testHook(useSubscriptionModels)(initial);
97+
98+
act(() => {
99+
renderResult.result.current.setEditLimitsTarget(2);
100+
});
101+
act(() => {
102+
renderResult.result.current.handleRemoveModel(0);
103+
});
104+
105+
expect(renderResult.result.current.editLimitsTarget).toBe(1);
106+
expect(renderResult.result.current.editingModel?.modelRefSummary.name).toBe('model-c');
107+
});
108+
109+
it('should clear editLimitsTarget when the edited row is removed', () => {
110+
const initial = [makeEntry('model-a'), makeEntry('model-b')];
111+
const renderResult = testHook(useSubscriptionModels)(initial);
112+
113+
act(() => {
114+
renderResult.result.current.setEditLimitsTarget(1);
115+
});
116+
act(() => {
117+
renderResult.result.current.handleRemoveModel(1);
118+
});
119+
120+
expect(renderResult.result.current.editLimitsTarget).toBeNull();
121+
expect(renderResult.result.current.editingModel).toBeNull();
122+
});
123+
124+
it('should adjust editLimitsTarget when removing multiple models by ref', () => {
125+
const initial = [makeEntry('model-a'), makeEntry('model-b'), makeEntry('model-c')];
126+
const renderResult = testHook(useSubscriptionModels)(initial);
127+
128+
act(() => {
129+
renderResult.result.current.setEditLimitsTarget(2);
130+
});
131+
act(() => {
132+
renderResult.result.current.handleRemoveModelsByRef([
133+
makeModelRef('model-a'),
134+
makeModelRef('model-b'),
135+
]);
136+
});
137+
138+
expect(renderResult.result.current.models).toHaveLength(1);
139+
expect(renderResult.result.current.editLimitsTarget).toBe(0);
140+
expect(renderResult.result.current.editingModel?.modelRefSummary.name).toBe('model-c');
141+
});
142+
79143
it('should remove models by ref', () => {
80144
const initial = [makeEntry('model-a'), makeEntry('model-b'), makeEntry('model-c')];
81145
const renderResult = testHook(useSubscriptionModels)(initial);
@@ -109,33 +173,23 @@ describe('useSubscriptionModels', () => {
109173
});
110174

111175
it('should report allModelsHaveRateLimits correctly', () => {
112-
const initial = [makeEntry('model-a', 'ns', [{ limit: 1000, window: '1h' }])];
113-
const renderResult = testHook(useSubscriptionModels)(initial);
176+
const renderResult = testHook(useSubscriptionModels)([
177+
makeEntry('model-a', 'ns', [{ limit: 1000, window: '1h' }]),
178+
]);
114179

115180
expect(renderResult.result.current.allModelsHaveRateLimits).toBe(true);
116181

117182
act(() => {
118183
renderResult.result.current.handleAddModels([makeModelRef('model-b')]);
119184
});
120185

186+
// false as soon as a model with no limits is added
121187
expect(renderResult.result.current.allModelsHaveRateLimits).toBe(false);
122-
});
123-
124-
it('should track rateLimitErrorIndices after closing the modal', () => {
125-
const initial = [makeEntry('model-a', 'ns', [])];
126-
const renderResult = testHook(useSubscriptionModels)(initial);
127-
128-
expect(renderResult.result.current.rateLimitErrorIndices.size).toBe(0);
129-
130-
act(() => {
131-
renderResult.result.current.setEditLimitsTarget(0);
132-
});
133-
act(() => {
134-
renderResult.result.current.handleCloseRateLimitsModal();
135-
});
188+
expect(renderResult.result.current.models).toHaveLength(2);
136189

137-
expect(renderResult.result.current.rateLimitErrorIndices.has(0)).toBe(true);
138-
expect(renderResult.result.current.editLimitsTarget).toBeNull();
190+
// also false when initialized with a model that already has no limits
191+
const renderResult2 = testHook(useSubscriptionModels)([makeEntry('model-a', 'ns', [])]);
192+
expect(renderResult2.result.current.allModelsHaveRateLimits).toBe(false);
139193
});
140194

141195
it('should return the editingModel when editLimitsTarget is set', () => {

‎packages/maas/frontend/src/app/hooks/useSubscriptionModels.ts‎

Lines changed: 32 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,27 @@ import {
55
TokenRateLimit,
66
} from '~/app/types/subscriptions';
77

8+
/** Returns the corrected edit-modal row index after deletions, or null if that row was removed. */
9+
export const reindexAfterRemove = (
10+
targetIndex: number | null,
11+
removedIndices: number[],
12+
): number | null => {
13+
if (targetIndex === null || removedIndices.length === 0) {
14+
return targetIndex;
15+
}
16+
if (removedIndices.includes(targetIndex)) {
17+
return null;
18+
}
19+
return targetIndex - removedIndices.filter((i) => i < targetIndex).length;
20+
};
21+
822
type UseSubscriptionModelsReturn = {
923
models: SubscriptionModelEntry[];
1024
isAddModelsModalOpen: boolean;
1125
setIsAddModelsModalOpen: React.Dispatch<React.SetStateAction<boolean>>;
1226
editLimitsTarget: number | null;
1327
setEditLimitsTarget: React.Dispatch<React.SetStateAction<number | null>>;
1428
editingModel: SubscriptionModelEntry | null;
15-
rateLimitErrorIndices: Set<number>;
1629
allModelsHaveRateLimits: boolean;
1730
handleAddModels: (refs: MaaSModelRefSummary[]) => void;
1831
handleRemoveModel: (index: number) => void;
@@ -27,23 +40,9 @@ export const useSubscriptionModels = (
2740
const [models, setModels] = React.useState<SubscriptionModelEntry[]>(initialModels);
2841
const [isAddModelsModalOpen, setIsAddModelsModalOpen] = React.useState(false);
2942
const [editLimitsTarget, setEditLimitsTarget] = React.useState<number | null>(null);
30-
const [rateLimitsTouched, setRateLimitsTouched] = React.useState<Set<number>>(new Set());
3143

3244
const allModelsHaveRateLimits = models.every((m) => m.tokenRateLimits.length > 0);
3345

34-
const rateLimitErrorIndices = React.useMemo(
35-
() =>
36-
new Set(
37-
models.reduce<number[]>((acc, m, i) => {
38-
if (rateLimitsTouched.has(i) && m.tokenRateLimits.length === 0) {
39-
acc.push(i);
40-
}
41-
return acc;
42-
}, []),
43-
),
44-
[models, rateLimitsTouched],
45-
);
46-
4746
const editingModel = editLimitsTarget != null ? models[editLimitsTarget] : null;
4847

4948
const handleAddModels = React.useCallback((selectedRefs: MaaSModelRefSummary[]) => {
@@ -63,43 +62,27 @@ export const useSubscriptionModels = (
6362

6463
const handleRemoveModel = React.useCallback((index: number) => {
6564
setModels((prev) => prev.filter((_, i) => i !== index));
66-
setRateLimitsTouched((prev) => {
67-
const next = new Set<number>();
68-
prev.forEach((i) => {
69-
if (i < index) {
70-
next.add(i);
71-
} else if (i > index) {
72-
next.add(i - 1);
73-
}
74-
});
75-
return next;
76-
});
65+
setEditLimitsTarget((current) => reindexAfterRemove(current, [index]));
7766
}, []);
7867

79-
const handleRemoveModelsByRef = React.useCallback((refs: MaaSModelRefSummary[]) => {
80-
const keysToRemove = new Set(refs.map((r) => `${r.namespace}/${r.name}`));
81-
setModels((prev) => {
82-
const removedIndices = new Set<number>();
83-
prev.forEach((m, i) => {
68+
const handleRemoveModelsByRef = React.useCallback(
69+
(refs: MaaSModelRefSummary[]) => {
70+
const keysToRemove = new Set(refs.map((r) => `${r.namespace}/${r.name}`));
71+
const removedIndices = models.reduce<number[]>((acc, m, i) => {
8472
if (keysToRemove.has(`${m.modelRefSummary.namespace}/${m.modelRefSummary.name}`)) {
85-
removedIndices.add(i);
86-
}
87-
});
88-
setRateLimitsTouched((prevTouched) => {
89-
const next = new Set<number>();
90-
let offset = 0;
91-
for (let i = 0; i < prev.length; i++) {
92-
if (removedIndices.has(i)) {
93-
offset++;
94-
} else if (prevTouched.has(i)) {
95-
next.add(i - offset);
96-
}
73+
acc.push(i);
9774
}
98-
return next;
99-
});
100-
return prev.filter((_, i) => !removedIndices.has(i));
101-
});
102-
}, []);
75+
return acc;
76+
}, []);
77+
setModels((prev) =>
78+
prev.filter(
79+
(m) => !keysToRemove.has(`${m.modelRefSummary.namespace}/${m.modelRefSummary.name}`),
80+
),
81+
);
82+
setEditLimitsTarget((current) => reindexAfterRemove(current, removedIndices));
83+
},
84+
[models],
85+
);
10386

10487
const handleSaveRateLimits = React.useCallback(
10588
(rateLimits: TokenRateLimit[]) => {
@@ -116,14 +99,8 @@ export const useSubscriptionModels = (
11699
);
117100

118101
const handleCloseRateLimitsModal = React.useCallback(() => {
119-
setRateLimitsTouched((prev) => {
120-
if (editLimitsTarget == null) {
121-
return prev;
122-
}
123-
return new Set(prev).add(editLimitsTarget);
124-
});
125102
setEditLimitsTarget(null);
126-
}, [editLimitsTarget]);
103+
}, []);
127104

128105
return {
129106
models,
@@ -132,7 +109,6 @@ export const useSubscriptionModels = (
132109
editLimitsTarget,
133110
setEditLimitsTarget,
134111
editingModel,
135-
rateLimitErrorIndices,
136112
allModelsHaveRateLimits,
137113
handleAddModels,
138114
handleRemoveModel,

‎packages/maas/frontend/src/app/pages/subscriptions/createSubscription/CreateSubscriptionForm.tsx‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,6 @@ const CreateSubscriptionForm: React.FC<CreateSubscriptionFormProps> = ({
113113
editLimitsTarget,
114114
setEditLimitsTarget,
115115
editingModel,
116-
rateLimitErrorIndices,
117116
allModelsHaveRateLimits,
118117
handleAddModels,
119118
handleRemoveModel,
@@ -375,7 +374,6 @@ const CreateSubscriptionForm: React.FC<CreateSubscriptionFormProps> = ({
375374
tokenRateLimits: m.tokenRateLimits,
376375
}))}
377376
editable
378-
rateLimitErrorIndices={rateLimitErrorIndices}
379377
onAddModels={canAddModels ? () => setIsAddModelsModalOpen(true) : undefined}
380378
onEditLimits={(index) => setEditLimitsTarget(index)}
381379
onRemoveModel={handleRemoveModel}

‎packages/maas/frontend/src/app/shared/MaasModelsSection.tsx‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
Flex,
1010
FlexItem,
1111
FormGroup,
12+
FormHelperText,
1213
HelperText,
1314
HelperTextItem,
1415
MenuToggle,
@@ -37,7 +38,6 @@ export type MaasModelsSectionProps = {
3738
titleHeadingLevel?: React.ComponentProps<typeof Title>['headingLevel'];
3839
titleSize?: React.ComponentProps<typeof Title>['size'];
3940
editable?: boolean;
40-
rateLimitErrorIndices?: Set<number>;
4141
onAddModels?: () => void;
4242
onEditLimits?: (index: number) => void;
4343
onRemoveModel?: (index: number) => void;
@@ -57,7 +57,6 @@ const MaasModelsSection: React.FC<MaasModelsSectionProps> = ({
5757
titleHeadingLevel = 'h2',
5858
titleSize = 'xl',
5959
editable = false,
60-
rateLimitErrorIndices,
6160
onAddModels,
6261
onEditLimits,
6362
onRemoveModel,
@@ -135,13 +134,13 @@ const MaasModelsSection: React.FC<MaasModelsSectionProps> = ({
135134
</Button>
136135
</StackItem>
137136
<StackItem>
138-
<HelperText>
139-
<HelperTextItem
140-
variant={rateLimitErrorIndices?.has(index) ? 'error' : 'indeterminate'}
141-
>
142-
At least one token limit is required
143-
</HelperTextItem>
144-
</HelperText>
137+
<FormHelperText>
138+
<HelperText>
139+
<HelperTextItem variant="error">
140+
At least one token limit is required
141+
</HelperTextItem>
142+
</HelperText>
143+
</FormHelperText>
145144
</StackItem>
146145
</Stack>
147146
)

0 commit comments

Comments
 (0)