Skip to content

Commit 7627de0

Browse files
committed
Fix review findings across summary API and resource cards
- Fix broken reactivity in useDefaultResources (toValue inside computed) - Fix division by zero in StatusCard percent function - Fix race condition in useResourceSummary with fetchId staleness guard - Fix inconsistent color assignment in StatusCard rowAccumulator - Fix inconsistent sort comparator in useResourceCardRowFromRelationships - Add color fallback for missing stateSimpleColor in StatusCard - Add error handling for fire-and-forget async in useResourceSummary - Parallelize service/ingress summary fetches with Promise.all - Rename opt.namespaced to opt.namespace for clarity - Replace hardcoded 'Resources' with i18n key - Extract duplicated 'metadata.state.name' to constant - Use nullish coalescing for res.count - Fix JSDoc example to include required summaryField
1 parent ded904c commit 7627de0

5 files changed

Lines changed: 55 additions & 30 deletions

File tree

shell/components/Resource/Detail/Card/StateCard/composables.ts

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,11 @@ export function useResourceCardRowFromRelationships(label: string, relationships
163163
return 1;
164164
}
165165

166-
return left.count >= right.count ? -1 : 1;
166+
if (left.count === right.count) {
167+
return 0;
168+
}
169+
170+
return left.count > right.count ? -1 : 1;
167171
});
168172

169173
return {
@@ -181,8 +185,7 @@ export interface Pairs {
181185
}
182186

183187
export function useDefaultResources(pairs: Ref<Pairs[]>) {
184-
const pairsValue = toValue(pairs);
185-
const rows = computed(() => pairsValue.map(({ label, resources, to }) => useResourceCardRow(label, resources, undefined, undefined, to)));
188+
const rows = computed(() => toValue(pairs).map(({ label, resources, to }) => useResourceCardRow(label, resources, undefined, undefined, to)));
186189

187190
return rows;
188191
}
@@ -236,7 +239,7 @@ export function useDefaultWorkloadResources(services?: any[], ingresses?: any[],
236239
const rows = useDefaultResources(remainingPairs);
237240

238241
return {
239-
title: 'Resources',
242+
title: i18n.t('component.resource.detail.card.resourcesCard.title'),
240243
rows: rows.value
241244
};
242245
}
@@ -268,7 +271,7 @@ export function useDefaultWorkloadInsightsCardProps(): StateCardProps {
268271

269272
export interface ResourceSummaryOpt {
270273
summaryField: string;
271-
namespaced?: string;
274+
namespace?: string;
272275
filters?: PaginationParamFilter[];
273276
}
274277

@@ -284,13 +287,24 @@ export function useResourceSummary(type: string, opt: ResourceSummaryOpt) {
284287
const summary = ref<SummaryResult['summary']>(null);
285288

286289
const normalizedType = store.getters['cluster/normalizeType']?.(type) || type;
290+
let fetchId = 0;
287291

288292
async function fetch() {
289-
const result = await store.dispatch('cluster/fetchResourceSummary', { type, opt });
293+
const id = ++fetchId;
294+
295+
try {
296+
const result = await store.dispatch('cluster/fetchResourceSummary', { type, opt });
297+
298+
if (id !== fetchId) {
299+
return;
300+
}
290301

291-
if (result) {
292-
count.value = result.count;
293-
summary.value = result.summary;
302+
if (result) {
303+
count.value = result.count;
304+
summary.value = result.summary;
305+
}
306+
} catch (e) {
307+
console.warn(`useResourceSummary: fetch failed for type "${ type }"`, e); // eslint-disable-line no-console
294308
}
295309
}
296310

shell/components/Resource/Detail/Card/StatusCard/index.vue

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ const segmentAccumulator = computed(() => {
6161
}
6262
} else {
6363
props.resources?.forEach((resource: any) => {
64-
const color: StateColor = resource.stateSimpleColor;
64+
const color: StateColor = resource.stateSimpleColor || 'disabled';
6565
6666
accumulator[color] = accumulator[color] || { count: 0 };
6767
accumulator[color].count++;
@@ -90,17 +90,18 @@ const rowAccumulator = computed(() => {
9090
}
9191
} else {
9292
props.resources?.forEach((resource: any) => {
93-
accumulator[resource.stateDisplay] = accumulator[resource.stateDisplay] || { count: 0, color: 'disabled' as StateColor };
93+
const color = (resource.stateSimpleColor?.replace('text-', '') || 'disabled') as StateColor;
94+
95+
accumulator[resource.stateDisplay] = accumulator[resource.stateDisplay] || { count: 0, color };
9496
accumulator[resource.stateDisplay].count++;
95-
accumulator[resource.stateDisplay].color = resource.stateSimpleColor.replace('text-', '') as StateColor;
9697
});
9798
}
9899
99100
return accumulator;
100101
});
101102
102103
const percent = (count: number, total: number) => {
103-
return count / total * 100;
104+
return total > 0 ? count / total * 100 : 0;
104105
};
105106
106107
const count = computed(() => {

shell/models/workload.js

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,7 @@ export default class Workload extends WorkloadService {
590590
async fetchSummaries() {
591591
const ns = this.metadata.namespace;
592592
const summaries = {};
593+
const summaryField = 'metadata.state.name';
593594
const nsFilter = PaginationParamFilter.createSingleField({ field: 'metadata.namespace', value: ns });
594595

595596
const podSelectorStr = this.podSelector;
@@ -608,18 +609,24 @@ export default class Workload extends WorkloadService {
608609
}
609610
});
610611

611-
summaries.pods = await this.$dispatch('fetchResourceSummary', { type: POD, opt: { summaryField: 'metadata.state.name', filters } });
612+
summaries.pods = await this.$dispatch('fetchResourceSummary', { type: POD, opt: { summaryField, filters } });
612613
}
613614

614-
summaries.services = await this.$dispatch('fetchResourceSummary', {
615-
type: SERVICE,
616-
opt: { summaryField: 'metadata.state.name', filters: [nsFilter] }
617-
});
618-
619-
summaries.ingresses = await this.$dispatch('fetchResourceSummary', {
620-
type: INGRESS,
621-
opt: { summaryField: 'metadata.state.name', filters: [nsFilter] }
622-
});
615+
// Service and ingress summaries are scoped by namespace only (not by workload selector)
616+
// because the relationship between workloads and services/ingresses is not label-based.
617+
const [services, ingresses] = await Promise.all([
618+
this.$dispatch('fetchResourceSummary', {
619+
type: SERVICE,
620+
opt: { summaryField, filters: [nsFilter] }
621+
}),
622+
this.$dispatch('fetchResourceSummary', {
623+
type: INGRESS,
624+
opt: { summaryField, filters: [nsFilter] }
625+
}),
626+
]);
627+
628+
summaries.services = services;
629+
summaries.ingresses = ingresses;
623630

624631
set(this, '_summaries', summaries);
625632

shell/plugins/steve/__tests__/actions.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ describe('steve: actions:', () => {
7878

7979
ctx.dispatch.mockResolvedValue({ count: 2, summary: null });
8080

81-
await fetchResourceSummary.call({}, ctx, { type: 'pod', opt: { summaryField: 'metadata.state.name', namespaced: 'cattle-system' } });
81+
await fetchResourceSummary.call({}, ctx, { type: 'pod', opt: { summaryField: 'metadata.state.name', namespace: 'cattle-system' } });
8282

8383
const requestUrl = ctx.dispatch.mock.calls[0][1].opt.url;
8484

@@ -92,7 +92,7 @@ describe('steve: actions:', () => {
9292
ctx.getters.schemaFor = () => nonNsSchema;
9393
ctx.dispatch.mockResolvedValue({ count: 1, summary: null });
9494

95-
await fetchResourceSummary.call({}, ctx, { type: 'pod', opt: { summaryField: 'metadata.state.name', namespaced: 'default' } });
95+
await fetchResourceSummary.call({}, ctx, { type: 'pod', opt: { summaryField: 'metadata.state.name', namespace: 'default' } });
9696

9797
const requestUrl = ctx.dispatch.mock.calls[0][1].opt.url;
9898

shell/plugins/steve/actions.js

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -230,13 +230,16 @@ export default {
230230
* @param {object} [opt] - Options object
231231
* @param {string} opt.summaryField - Field to aggregate counts by.
232232
* Must be a field indexed by the VAI cache (see StevePaginationUtils.VALID_FIELDS in steve-pagination-utils.ts)
233-
* @param {string} [opt.namespaced] - Namespace to scope the request to (only applies to namespaced resource types)
233+
* @param {string} [opt.namespace] - Namespace to scope the request to (only applies to namespaced resource types)
234234
* @param {PaginationParamFilter[]} [opt.filters] - Pre-built filters from PaginationParamFilter.createSingleField()
235235
* @returns {Promise<{ count: number, summary: { property: string, counts: Record<string, number> }[] | null } | undefined>}
236236
*
237237
* @example
238238
* const nsFilter = PaginationParamFilter.createSingleField({ field: 'metadata.namespace', value: 'default' });
239-
* const result = await dispatch('fetchResourceSummary', { type: 'pod', opt: { filters: [nsFilter] } });
239+
* const result = await dispatch('fetchResourceSummary', {
240+
* type: 'pod',
241+
* opt: { summaryField: 'metadata.state.name', filters: [nsFilter] }
242+
* });
240243
*/
241244
async fetchResourceSummary({ getters, dispatch, rootGetters }, { type, opt = {} }) {
242245
type = getters.normalizeType(type);
@@ -263,8 +266,8 @@ export default {
263266
try {
264267
const url = new URL(schema.links.collection, window.location.origin);
265268

266-
if (schema.attributes?.namespaced && opt.namespaced) {
267-
url.pathname += `/${ opt.namespaced }`;
269+
if (schema.attributes?.namespaced && opt.namespace) {
270+
url.pathname += `/${ opt.namespace }`;
268271
}
269272

270273
url.searchParams.set('summary', opt.summaryField);
@@ -281,7 +284,7 @@ export default {
281284
const res = await dispatch('request', { opt: { url: url.pathname + url.search } });
282285

283286
return {
284-
count: res.count || 0,
287+
count: res.count ?? 0,
285288
summary: res.summary || null
286289
};
287290
} catch (e) {

0 commit comments

Comments
 (0)