Copilot commented on code in PR #3317: URL: https://github.com/apache/apisix-dashboard/pull/3317#discussion_r2903079829
########## src/components/page/ResourceListPage.tsx: ########## @@ -0,0 +1,139 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import type { ProColumns } from '@ant-design/pro-components'; +import { ProTable } from '@ant-design/pro-components'; +import type { TablePaginationConfig } from 'antd'; +import { useMemo } from 'react'; +import { useTranslation } from 'react-i18next'; + +import { AntdConfigProvider } from '@/config/antdConfigProvider'; + +import PageHeader from './PageHeader'; +import type { ToAddPageBtnProps } from './ToAddPageBtn'; +import { ToAddPageBtn } from './ToAddPageBtn'; + +interface ResourceListPageProps<T> { + titleKey?: string; + columns: ProColumns<T>[]; + queryData: { + data?: { list: T[]; total: number } | undefined; + isLoading: boolean; + pagination: TablePaginationConfig | false; + refetch?: () => void; + }; Review Comment: `queryData.refetch` is part of `ResourceListPageProps`, but `ResourceListPage` never uses it. This makes the API surface larger than necessary and can mislead callers into thinking the component will refetch on its own. Suggestion: remove `refetch` from the prop type (callers can keep using `refetch` inside column renderers), or add a concrete use for it (e.g., a refresh action in the toolbar) so it’s not dead API. ########## src/apis/upstreams.ts: ########## @@ -77,6 +77,15 @@ export const deleteAllUpstreams = async (req: AxiosInstance) => { req.delete(`${API_UPSTREAMS}/${d.value.id}`).catch((err) => { // Ignore 404 errors as the resource might have been deleted if (axios.isAxiosError(err) && err.response?.status === 404) return; + // Ignore 400 errors only when the upstream is still referenced by routes/services + if ( + axios.isAxiosError(err) && + err.response?.status === 400 && + typeof err.response?.data?.error_msg === 'string' && + err.response.data.error_msg.includes('still being used') + ) { + return; + } Review Comment: `deleteAllUpstreams` now silently ignores HTTP 400 responses when the error message contains "still being used". This can leave upstreams behind while the caller assumes the environment is clean (e.g. list/pagination tests that rely on an empty baseline), and it also makes failures harder to diagnose. Consider keeping the default behavior strict (throw on 400), and/or returning/logging the IDs that could not be deleted (or accepting an explicit `ignoreInUse`/`bestEffort` option so callers opt into this behavior). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
