Copilot commented on code in PR #493:
URL:
https://github.com/apache/skywalking-booster-ui/pull/493#discussion_r2284833484
##########
src/hooks/useAssociateProcessor.ts:
##########
@@ -32,7 +32,8 @@ export default function useAssociateProcessor(props:
AssociateProcessorProps) {
return;
}
const list = props.option.series[0].data.map((d: (number | string)[]) =>
d[0]);
- if (!list.includes(props.filters.duration.endTime)) {
+ const { startTime, endTime } = props.filters.duration || {};
+ if (!list.includes(endTime)) {
Review Comment:
The code destructures `startTime` and `endTime` from `props.filters.duration
|| {}` but only checks if `endTime` is in the list. If `props.filters.duration`
is falsy, `endTime` will be undefined, and `list.includes(undefined)` will
likely return false. This could cause unexpected behavior when the duration
object is missing but the function should still process the data.
```suggestion
if (typeof endTime === "undefined" || !list.includes(endTime)) {
```
##########
src/views/dashboard/graphs/EndpointList.vue:
##########
@@ -165,7 +165,14 @@ limitations under the License. -->
{ metricConfig: metricConfig.value || [], expressions, subExpressions
},
EntityType[2].value,
);
- currentEndpoints.value = params.data;
+ currentEndpoints.value = params.data
+ .filter((item): item is Endpoint => "merge" in item && typeof
item.merge === "string")
+ .map((item) => ({
+ id: item.id,
+ value: item.value,
+ label: item.label,
+ merge: item.merge,
+ }));
Review Comment:
The type guard checks for the existence of a `merge` property and ensures
it's a string, but this seems like a workaround for type misalignment. The
filtering and mapping logic that follows suggests the data structure returned
from `params.data` doesn't match the expected `Endpoint` type. Consider fixing
the root cause by properly typing the data flow.
```suggestion
// Assume params.data is now properly typed as Endpoint[]
currentEndpoints.value = params.data.map((item: Endpoint) => ({
id: item.id,
value: item.value,
label: item.label,
merge: item.merge,
}));
```
##########
src/views/dashboard/graphs/ServiceList.vue:
##########
@@ -219,11 +219,11 @@ limitations under the License. -->
if (expressions.length && expressions[0]) {
const params = await useExpressionsQueryPodsMetrics(
- currentServices,
+ currentServices as unknown as PodWithMetrics[],
{ metricConfig: metricConfig.value || [], expressions, subExpressions
},
EntityType[0].value,
);
- services.value = params.data;
+ services.value = params.data as unknown as ServiceWithGroup[];
Review Comment:
Another instance of unsafe double type assertion `as unknown as
ServiceWithGroup[]`. This pattern is repeated and suggests a fundamental type
mismatch between the expected return type and the actual data structure.
Consider creating proper type mappings or adapter functions instead of using
type assertions.
```suggestion
services.value = mapToServiceWithGroup(params.data);
```
##########
src/hooks/useExpressionsProcessor.ts:
##########
@@ -369,19 +442,30 @@ export async function useExpressionsQueryPodsMetrics(
return { data, names, subNames, metricConfigArr, metricTypesArr,
expressionsTips, subExpressionsTips };
}
- async function fetchPodsExpressionValues(pods: Array<(Instance | Endpoint |
Service) & Indexable>) {
+ async function fetchPodsExpressionValues(pods: Array<PodWithMetrics>):
Promise<ExpressionsPodsSourceResult> {
const dashboardStore = useDashboardStore();
const params = await expressionsGraphqlPods(pods);
const json = await dashboardStore.fetchMetricValue(
params as { queryStr: string; conditions: { [key: string]: unknown } },
);
- if (json.errors) {
+ if ((json as { errors?: string }).errors) {
ElMessage.error(json.errors);
Review Comment:
The type assertion `(json as { errors?: string }).errors` is used to check
for errors, but then `json.errors` is accessed directly on the next line
without the type assertion. This inconsistency makes the code harder to follow
and maintain. Consider using a consistent approach throughout the function.
##########
src/views/dashboard/graphs/ServiceList.vue:
##########
@@ -219,11 +219,11 @@ limitations under the License. -->
if (expressions.length && expressions[0]) {
const params = await useExpressionsQueryPodsMetrics(
- currentServices,
+ currentServices as unknown as PodWithMetrics[],
Review Comment:
Using double type assertion with `as unknown as PodWithMetrics[]` is a code
smell that suggests the types may not be properly aligned. This bypasses
TypeScript's type checking and could hide potential runtime errors. Consider
refactoring the types to avoid the need for this unsafe casting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
