Copilot commented on code in PR #3305: URL: https://github.com/apache/apisix-dashboard/pull/3305#discussion_r2897247942
########## src/components/page/ResourceListPage.tsx: ########## @@ -0,0 +1,140 @@ +/** + * 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; + }; + rowKey: string | ((record: T) => string); + addPageTo?: ToAddPageBtnProps['to']; + resourceNameKey?: string; + emptyText?: React.ReactNode; + pageSizeOptions?: number[] | string[]; + showTotal?: (total: number, range: [number, number]) => React.ReactNode; +} + +function isRecord(val: unknown): val is Record<string, unknown> { + return val !== null && typeof val === 'object' && !Array.isArray(val); +} + +const ResourceListPage = <T extends Record<string, unknown>>( + props: ResourceListPageProps<T>, +) => { + const { + titleKey, + columns, + queryData, + rowKey, + addPageTo, + resourceNameKey, + emptyText, + pageSizeOptions, + showTotal: customShowTotal, + } = props; + const { t } = useTranslation(); + const { data, isLoading, pagination } = queryData; + + const dataSource = useMemo(() => data?.list ?? [], [data]); + const total = + (pagination !== false ? pagination?.total : undefined) ?? data?.total ?? 0; + + const paginationConfig = useMemo(() => { + if (pagination === false) return false; + + return { + current: pagination?.current ?? 1, + pageSize: pagination?.pageSize ?? 10, + total, + showSizeChanger: true, + pageSizeOptions: pageSizeOptions ?? [10, 20, 50, 100], + hideOnSinglePage: false, + onChange: pagination?.onChange, + ...(customShowTotal ? { showTotal: customShowTotal } : {}), + }; + }, [pagination, total, pageSizeOptions, customShowTotal]); + + const resolvedRowKey = useMemo( + () => + typeof rowKey === 'function' + ? rowKey + : (record: T) => { + // APISIX list items wrap data in a `value` property + const raw = record['value']; + const value = isRecord(raw) ? raw : undefined; + return String(value?.[rowKey] ?? record[rowKey] ?? record['key'] ?? ''); + }, Review Comment: `resolvedRowKey` falls back to `''` when it can’t find `value[rowKey]`, `record[rowKey]`, or `record.key`. If that happens, multiple rows will share the same key, which can cause React key warnings and unstable row rendering. Consider using the `index` argument of the rowKey function as a final fallback (or throwing/logging when a key can’t be resolved) to ensure a stable, unique key per row. ########## src/routes/services/detail.$id/stream_routes/index.tsx: ########## @@ -55,6 +50,8 @@ export const Route = createFileRoute('/services/detail/$id/stream_routes/')({ errorComponent: StreamRoutesErrorComponent, validateSearch: pageSearchSchema, loaderDeps: ({ search }) => search, - loader: ({ deps }) => - queryClient.ensureQueryData(getStreamRouteListQueryOptions(deps)), -}); + loader: ({ deps, params: { id } }) => + queryClient.ensureQueryData( + getStreamRouteListQueryOptions({ ...deps, filter: { service_id: id } }), + ), Review Comment: The route loader overwrites any existing `deps.filter` by setting `filter: { service_id: id }`. Since `pageSearchSchema` is `.passthrough()`, additional filter fields could be present in `deps` and would be lost during prefetching; this can also create a mismatch between loader-prefetched data and what `useStreamRouteList(..., defaultParams)` fetches. Consider merging with any existing filter while still enforcing `service_id`. ########## package.json: ########## @@ -102,11 +102,12 @@ "overrides": { "lodash": ">=4.17.21", "minimatch": ">=3.0.5", - "@swc/core": "1.10.0" + "@swc/core": "1.10.0", + "@iconify/utils": "2.1.33" }, Review Comment: `pnpm.overrides` forces `@iconify/utils` to `2.1.33`, while `unplugin-icons` is being installed with that downgraded version (see pnpm-lock entry for `[email protected]`). Since this is a major-version downgrade, it risks runtime/build incompatibilities in the Vite icons plugin. If this pin is required (e.g., for a security advisory), consider adding a brief rationale (and/or a compatibility note) and ensure it’s aligned with `unplugin-icons`’ expected `@iconify/utils` major version. ########## src/routes/services/detail.$id/routes/index.tsx: ########## @@ -15,44 +15,41 @@ * limitations under the License. */ import { createFileRoute, useParams } from '@tanstack/react-router'; -import { useTranslation } from 'react-i18next'; import { getRouteListQueryOptions } from '@/apis/hooks'; -import PageHeader from '@/components/page/PageHeader'; import { ToDetailPageBtn } from '@/components/page/ToAddPageBtn'; import { queryClient } from '@/config/global'; import { RouteList } from '@/routes/routes'; import { pageSearchSchema } from '@/types/schema/pageSearch'; function RouteComponent() { - const { t } = useTranslation(); const { id } = useParams({ from: '/services/detail/$id/routes/' }); return ( - <> - <PageHeader title={t('sources.routes')} /> - <RouteList - routeKey="/services/detail/$id/routes/" - defaultParams={{ - filter: { - service_id: id, - }, - }} - ToDetailBtn={({ record }) => ( - <ToDetailPageBtn - key="detail" - to="/services/detail/$id/routes/detail/$routeId" - params={{ id, routeId: record.value.id }} - /> - )} - /> - </> + <RouteList + titleKey="sources.routes" + routeKey="/services/detail/$id/routes/" + defaultParams={{ + filter: { + service_id: id, + }, + }} + ToDetailBtn={({ record }) => ( + <ToDetailPageBtn + key="detail" + to="/services/detail/$id/routes/detail/$routeId" + params={{ id, routeId: record.value.id }} + /> + )} + /> ); } export const Route = createFileRoute('/services/detail/$id/routes/')({ component: RouteComponent, validateSearch: pageSearchSchema, loaderDeps: ({ search }) => search, - loader: ({ deps }) => - queryClient.ensureQueryData(getRouteListQueryOptions(deps)), -}); + loader: ({ deps, params: { id } }) => + queryClient.ensureQueryData( + getRouteListQueryOptions({ ...deps, filter: { service_id: id } }), + ), Review Comment: The route loader overwrites any existing `deps.filter` by setting `filter: { service_id: id }`. If other filter fields are ever added/passed via the URL (pageSearchSchema is `.passthrough()`), they’ll be dropped for prefetching, and loader-prefetched data can diverge from what the list hook fetches. Prefer merging with any existing filter (e.g., preserve `deps.filter` while enforcing `service_id`). ########## e2e/utils/test.ts: ########## @@ -24,50 +24,87 @@ import { env } from './env'; export type Test = typeof test; export const test = baseTest.extend<object, { workerStorageState: string }>({ - storageState: ({ workerStorageState }, use) => use(workerStorageState), + storageState: ({ workerStorageState }, use) => { + // If storageState file doesn't exist yet, use undefined to skip loading it + // Playwright will still save it after tests run if needed + return use(workerStorageState); + }, Review Comment: The comment says the fixture will use `undefined` storageState if the auth file doesn’t exist yet, but the implementation always passes `workerStorageState` to `use(...)`. Either update the comment to match current behavior or add the existence check if skipping storageState loading is intended. -- 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]
