Copilot commented on code in PR #3305:
URL: https://github.com/apache/apisix-dashboard/pull/3305#discussion_r2872374064
##########
src/routes/services/detail.$id/routes/index.tsx:
##########
@@ -55,4 +50,4 @@ export const Route =
createFileRoute('/services/detail/$id/routes/')({
loaderDeps: ({ search }) => search,
loader: ({ deps }) =>
queryClient.ensureQueryData(getRouteListQueryOptions(deps)),
-});
+});
Review Comment:
The route loader prefetches `getRouteListQueryOptions(deps)` without the
`service_id` filter, but the page renders `RouteList` with
`defaultParams.filter.service_id`. This means the prefetched query key/data
won’t match the suspense query and can briefly fetch/show the unfiltered list
(and wastes the prefetch). Align the loader with the component by including
`filter: { service_id: id }` (as done for service stream_routes).
##########
src/types/schema/pageSearch.ts:
##########
@@ -19,16 +19,14 @@ import { z } from 'zod';
export const pageSearchSchema = z
.object({
- page: z
- .union([z.string(), z.number()])
- .optional()
- .default(1)
- .transform((val) => (val ? Number(val) : 1)),
- page_size: z
- .union([z.string(), z.number()])
- .optional()
- .default(10)
- .transform((val) => (val ? Number(val) : 10)),
+ page: z.preprocess(
+ (val) => (val === undefined || val === null ? undefined : Number(val)),
+ z.number().optional().default(1)
+ ),
+ page_size: z.preprocess(
Review Comment:
`Number(val)` coercion will accept `0` (and turns an empty string into `0`),
so `page=0` can now pass validation and yield `current: 0` in AntD pagination.
Consider coercing only non-empty values and adding `int().min(1)` (and
similarly for `page_size`) to preserve prior behavior where falsy/0 defaulted
to 1/10.
##########
e2e/server/apisix_conf.yml:
##########
@@ -1,42 +1,22 @@
-#
-# 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.
-#
-
apisix:
- node_listen: 9080 # APISIX listening port
+ node_listen: 9080
enable_ipv6: false
Review Comment:
The ASF license header was removed from this config file, but other files
under `e2e/server/` (e.g. `docker-compose.yml`) still include it. To stay
consistent with repository licensing conventions, please restore the header
here as well.
##########
e2e/utils/test.ts:
##########
@@ -43,31 +44,45 @@ export const test = baseTest.extend<object, {
workerStorageState: string }>({
return use(fileName);
}
- const page = await browser.newPage({ storageState: undefined });
+ try {
+ const page = await browser.newPage({ storageState: undefined });
+
+ // have to use env here, because the baseURL is not available in worker
+ await page.goto(env.E2E_TARGET_URL, { waitUntil: 'load' });
Review Comment:
`page` is created inside the `try`, but in the `catch` branch it won’t be
closed, which can leak pages/contexts and make subsequent tests flaky when auth
fails. Consider declaring `let page` outside and closing it in a `finally` (or
`await page?.close()` in the catch) before rethrowing.
##########
playwright.config.ts:
##########
@@ -24,10 +24,10 @@ import { env } from './e2e/utils/env';
export default defineConfig({
testDir: './e2e/tests',
outputDir: './test-results',
- fullyParallel: true,
+ fullyParallel: false,
forbidOnly: !!process.env.CI,
retries: process.env.CI ? 2 : 0,
- workers: process.env.CI ? 1 : undefined,
+ workers: 1,
Review Comment:
`fullyParallel: false` and `workers: 1` are now forced for all runs
(including local), which can significantly slow down E2E and conflicts with the
stated goal of improved parallel execution. If the intent is CI stability only,
consider gating these behind `process.env.CI` (or document why local
parallelism is intentionally disabled).
##########
e2e/tests/stream_routes.show-disabled-error.spec.ts:
##########
@@ -51,8 +51,7 @@ type APISIXConf = {
};
const getE2EServerDir = () => {
- const currentDir = new URL('.', import.meta.url).pathname;
- return path.join(currentDir, '../server');
+ return path.join(process.cwd(), 'e2e/server');
};
Review Comment:
Using `process.cwd()` makes this path dependent on how Playwright is invoked
(it can differ in some CI/devcontainer setups). For consistency with the
cross-platform fix used in `e2e/utils/common.ts`
(fileURLToPath(import.meta.url)), consider deriving the directory from
`import.meta.url` instead of the current working directory.
##########
src/components/page/ResourceListPage.tsx:
##########
@@ -0,0 +1,128 @@
+/**
+ * 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] ?? '');
+ },
+ [rowKey],
+ );
+
+ return (
+ <AntdConfigProvider>
+ {titleKey && <PageHeader title={t(titleKey, titleKey)} />}
+ <ProTable<T>
+ columns={columns}
+ dataSource={dataSource}
+ rowKey={resolvedRowKey}
+ loading={isLoading}
+ search={false}
+ options={false}
+ pagination={paginationConfig}
+ cardProps={{ bodyStyle: { padding: 0 } }}
+ locale={emptyText ? { emptyText } : undefined}
+ toolBarRender={() => [
+ addPageTo && resourceNameKey && (
+ <ToAddPageBtn
+ key="create"
+ to={addPageTo}
Review Comment:
This uses `toolBarRender` to render the “Add” action, but existing list
pages used `toolbar.menu` with `type: 'inline'` (e.g., credentials list still
does). If the goal is 1:1 UI behavior/standardization, consider using the same
`toolbar.menu` approach here (or refactor the remaining tables) so the Add
control lands in the same place and keeps consistent styling.
--
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]
