Copilot commented on code in PR #3305: URL: https://github.com/apache/apisix-dashboard/pull/3305#discussion_r2844900204
########## src/components/page/ResourceListPage.tsx: ########## @@ -0,0 +1,112 @@ +/** + * 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 type { APISIXListResponse } from '@/types/schema/apisix/type'; + +import PageHeader from './PageHeader'; +import { ToAddPageBtn } from './ToAddPageBtn'; + +interface ResourceListPageProps<T> { + titleKey?: string; + columns: ProColumns<T>[]; + queryHook: () => { + data?: APISIXListResponse<T> | undefined; + isLoading: boolean; + pagination: TablePaginationConfig | false; + refetch?: () => void; + }; + rowKey: string; + addPageTo?: string; + resourceNameKey?: string; + emptyKey?: string; +} + +const ResourceListPage = <T extends Record<string, unknown>>( + props: ResourceListPageProps<T> +) => { + const { + titleKey, + columns, + queryHook, + rowKey, + addPageTo, + resourceNameKey, + emptyKey, + } = props; + const { t } = useTranslation(); + const { data, isLoading, pagination } = queryHook(); + + const dataSource = useMemo(() => (data?.list as T[]) ?? [], [data]); + const total = pagination?.total ?? data?.total ?? 0; + + const paginationConfig = useMemo(() => { + if (pagination === false) return false; + + return { + current: pagination?.current ?? 1, + pageSize: pagination?.pageSize ?? 10, + total, + showSizeChanger: true, + pageSizeOptions: [10, 20, 50, 100], + hideOnSinglePage: false, + onChange: pagination?.onChange, + showTotal: (total: number, range: [number, number]) => + `${range[0]}-${range[1]} of ${total} items`, + }; + }, [pagination, total]); + + return ( + <> + {titleKey && <PageHeader title={t(titleKey as never)} />} + <ProTable<T> + columns={columns} + dataSource={dataSource} + rowKey={rowKey} + loading={isLoading} + search={false} + options={{ reload: true, density: true, setting: true }} Review Comment: The `options` prop is set to an object with reload, density, and setting features enabled, but the original implementations used `options={false}` to disable table options. This changes the user interface by adding table toolbar options that were not present before, deviating from the claimed "UI behavior, DOM structure, and user experience remain unchanged" in the PR description. ```suggestion options={false} ``` ########## src/components/page/ResourceListPage.tsx: ########## @@ -0,0 +1,112 @@ +/** + * 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 type { APISIXListResponse } from '@/types/schema/apisix/type'; + +import PageHeader from './PageHeader'; +import { ToAddPageBtn } from './ToAddPageBtn'; + +interface ResourceListPageProps<T> { + titleKey?: string; + columns: ProColumns<T>[]; + queryHook: () => { + data?: APISIXListResponse<T> | undefined; + isLoading: boolean; + pagination: TablePaginationConfig | false; + refetch?: () => void; + }; + rowKey: string; + addPageTo?: string; + resourceNameKey?: string; + emptyKey?: string; +} + +const ResourceListPage = <T extends Record<string, unknown>>( + props: ResourceListPageProps<T> +) => { + const { + titleKey, + columns, + queryHook, + rowKey, + addPageTo, + resourceNameKey, + emptyKey, + } = props; + const { t } = useTranslation(); + const { data, isLoading, pagination } = queryHook(); Review Comment: The `queryHook` is called directly in the component body (line 56) without being wrapped in a useMemo or useCallback. This means a new query hook instance is created on every render because the arrow function `() => ({ data, isLoading, pagination, refetch })` passed to queryHook in the route components creates a new reference each time. This will cause the query to be re-executed unnecessarily on every render, potentially leading to performance issues and infinite re-render loops. ########## src/routes/upstreams/index.tsx: ########## @@ -15,24 +15,22 @@ * limitations under the License. */ import type { ProColumns } from '@ant-design/pro-components'; -import { ProTable } from '@ant-design/pro-components'; import { createFileRoute } from '@tanstack/react-router'; import { useMemo } from 'react'; import { useTranslation } from 'react-i18next'; import { getUpstreamListQueryOptions, useUpstreamList } from '@/apis/hooks'; import { DeleteResourceBtn } from '@/components/page/DeleteResourceBtn'; -import PageHeader from '@/components/page/PageHeader'; -import { ToAddPageBtn, ToDetailPageBtn } from '@/components/page/ToAddPageBtn'; -import { AntdConfigProvider } from '@/config/antdConfigProvider'; +import ResourceListPage from '@/components/page/ResourceListPage'; +import { ToDetailPageBtn } from '@/components/page/ToAddPageBtn'; import { API_UPSTREAMS } from '@/config/constant'; import { queryClient } from '@/config/global'; import type { APISIXType } from '@/types/schema/apisix'; import { pageSearchSchema } from '@/types/schema/pageSearch'; -function RouteComponent() { +const RouteComponent = () => { const { t } = useTranslation(); - const { data, isLoading, refetch, pagination } = useUpstreamList(); + const { data, isLoading, pagination, refetch } = useUpstreamList(); Review Comment: The `refetch` value from the query hook is destructured but never used or passed to ResourceListPage. The original implementations passed `refetch` to action buttons in columns (e.g., DeleteResourceBtn). While the columns still receive refetch through closure, this inconsistency suggests the refetch mechanism might not work properly with the new component pattern, as there's no way to trigger refetch from the ResourceListPage component itself (e.g., through a refresh button). ########## src/apis/hooks.ts: ########## @@ -85,9 +90,10 @@ export const genUseList = < return (replaceKey?: U, defaultParams?: Partial<P>) => { const key = replaceKey || routeKey; const { params, setParams } = useSearchParams<T | U, P>(key); - const listQuery = useSuspenseQuery( - listQueryOptions({ ...defaultParams, ...params }) - ); + const listQuery = useQuery({ Review Comment: Changing from `useSuspenseQuery` to `useQuery` removes React Suspense support. This is a breaking change that affects how the application handles loading states and could cause issues with error boundaries and loading UI. The route loaders are being removed (as seen in services/index.tsx and routes/index.tsx), which means data is no longer being prefetched before navigation. This fundamentally changes the data loading behavior from suspense-based prefetching to on-demand loading, which contradicts the PR description claim that "UI behavior, DOM structure, and user experience remain unchanged". ```suggestion const listQuery = useSuspenseQuery({ ``` ########## src/components/page/ResourceListPage.tsx: ########## @@ -0,0 +1,112 @@ +/** + * 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 type { APISIXListResponse } from '@/types/schema/apisix/type'; + +import PageHeader from './PageHeader'; +import { ToAddPageBtn } from './ToAddPageBtn'; + +interface ResourceListPageProps<T> { + titleKey?: string; + columns: ProColumns<T>[]; + queryHook: () => { + data?: APISIXListResponse<T> | undefined; + isLoading: boolean; + pagination: TablePaginationConfig | false; + refetch?: () => void; + }; + rowKey: string; + addPageTo?: string; + resourceNameKey?: string; + emptyKey?: string; +} + +const ResourceListPage = <T extends Record<string, unknown>>( + props: ResourceListPageProps<T> +) => { + const { + titleKey, + columns, + queryHook, + rowKey, + addPageTo, + resourceNameKey, + emptyKey, + } = props; + const { t } = useTranslation(); + const { data, isLoading, pagination } = queryHook(); + + const dataSource = useMemo(() => (data?.list as T[]) ?? [], [data]); + const total = pagination?.total ?? data?.total ?? 0; + + const paginationConfig = useMemo(() => { + if (pagination === false) return false; + + return { + current: pagination?.current ?? 1, + pageSize: pagination?.pageSize ?? 10, + total, + showSizeChanger: true, + pageSizeOptions: [10, 20, 50, 100], + hideOnSinglePage: false, + onChange: pagination?.onChange, + showTotal: (total: number, range: [number, number]) => + `${range[0]}-${range[1]} of ${total} items`, + }; Review Comment: The pagination configuration hardcodes `pageSizeOptions: [10, 20, 50, 100]` and `showTotal` format. While this provides consistency, it removes flexibility for pages that might need different pagination options. Consider making these configurable through props for edge cases where different pagination behavior is needed. ########## src/routes/stream_routes/index.tsx: ########## @@ -158,7 +128,4 @@ export const Route = createFileRoute('/stream_routes/')({ component: StreamRouteComponent, errorComponent: StreamRoutesErrorComponent, validateSearch: pageSearchSchema, - loaderDeps: ({ search }) => search, - loader: ({ deps }) => - queryClient.ensureQueryData(getStreamRouteListQueryOptions(deps)), }); Review Comment: The route loader (loaderDeps and loader) has been removed, which means data is no longer prefetched during route navigation. Combined with the switch from useSuspenseQuery to useQuery, this changes the loading behavior from suspense-based prefetch to on-demand fetch after component mount. This can cause a flash of loading state that wasn't present before and contradicts the PR description claim of unchanged behavior. ########## src/types/schema/pageSearch.ts: ########## @@ -19,17 +19,16 @@ 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() + ), + page_size: z.preprocess( + (val) => (val === undefined || val === null ? undefined : Number(val)), + z.number().optional() + ), Review Comment: Removing default values for `page` and `page_size` in the schema means these values will be undefined if not provided in the URL. While the pagination config in ResourceListPage.tsx handles undefined with fallbacks (line 65-66: `pagination?.current ?? 1` and `pagination?.pageSize ?? 10`), this changes the URL behavior. Previously, navigating to `/services` would automatically add `?page=1&page_size=10` to the URL. Now these params won't appear unless explicitly set. This is a user-facing behavior change that contradicts the PR description. ########## src/components/page/ResourceListPage.tsx: ########## @@ -0,0 +1,112 @@ +/** + * 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 type { APISIXListResponse } from '@/types/schema/apisix/type'; + +import PageHeader from './PageHeader'; +import { ToAddPageBtn } from './ToAddPageBtn'; + +interface ResourceListPageProps<T> { + titleKey?: string; + columns: ProColumns<T>[]; + queryHook: () => { + data?: APISIXListResponse<T> | undefined; + isLoading: boolean; + pagination: TablePaginationConfig | false; + refetch?: () => void; + }; + rowKey: string; + addPageTo?: string; + resourceNameKey?: string; + emptyKey?: string; +} + +const ResourceListPage = <T extends Record<string, unknown>>( + props: ResourceListPageProps<T> +) => { + const { + titleKey, + columns, + queryHook, + rowKey, + addPageTo, + resourceNameKey, + emptyKey, + } = props; + const { t } = useTranslation(); + const { data, isLoading, pagination } = queryHook(); Review Comment: The ResourceListPage component doesn't handle query errors. The original implementations with useSuspenseQuery would bubble errors to error boundaries. Now with useQuery, errors are silently ignored and the component will just show a loading state indefinitely if the query fails. Consider adding error handling logic or at least accessing `error` and `isError` from the query hook result to properly handle failed queries. ########## e2e/tests/services.stream_routes.list.spec.ts: ########## @@ -16,240 +16,245 @@ */ import { servicesPom } from '@e2e/pom/services'; import { randomId } from '@e2e/utils/common'; +import { env } from '@e2e/utils/env'; import { e2eReq } from '@e2e/utils/req'; import { test } from '@e2e/utils/test'; import { uiGoto } from '@e2e/utils/ui'; -import { expect } from '@playwright/test'; +import { expect, type Page } from '@playwright/test'; -import { deleteAllServices, postServiceReq } from '@/apis/services'; -import { - deleteAllStreamRoutes, - postStreamRouteReq, -} from '@/apis/stream_routes'; +import { postServiceReq } from '@/apis/services'; +import { postStreamRouteReq } from '@/apis/stream_routes'; test.describe.configure({ mode: 'serial' }); const serviceName = randomId('test-service'); const anotherServiceName = randomId('another-service'); +// Use indexed octets with random offset to ensure uniqueness (0-4 starting at random value to avoid reserved ranges) +const randomOffset = Math.floor(Math.random() * 100) + 100; +let octetIndex = randomOffset; +const getUniqueOctet = () => (octetIndex++ % 256); const streamRoutes = [ { - server_addr: '127.0.0.1', + server_addr: `127.0.0.${getUniqueOctet()}`, server_port: 8080, }, { - server_addr: '127.0.0.2', + server_addr: `127.0.0.${getUniqueOctet()}`, server_port: 8081, }, { - server_addr: '127.0.0.3', + server_addr: `127.0.0.${getUniqueOctet()}`, server_port: 8082, }, ]; -// Stream route that uses upstream directly instead of service_id const upstreamStreamRoute = { - server_addr: '127.0.0.40', + server_addr: `127.0.0.${getUniqueOctet()}`, server_port: 9090, upstream: { nodes: [{ host: 'example.com', port: 80, weight: 100 }], }, }; -// Stream route that belongs to another service const anotherServiceStreamRoute = { - server_addr: '127.0.0.20', + server_addr: `127.0.0.${getUniqueOctet()}`, server_port: 9091, }; let testServiceId: string; let anotherServiceId: string; const createdStreamRoutes: string[] = []; -test.beforeAll(async () => { - await deleteAllStreamRoutes(e2eReq); - await deleteAllServices(e2eReq); +let upstreamStreamRouteId: string; +let anotherServiceStreamRouteId: string; - // Create a test service for testing service stream routes +test.beforeAll(async () => { const serviceResponse = await postServiceReq(e2eReq, { name: serviceName, desc: 'Test service for stream route listing', + labels: { test: serviceName }, }); testServiceId = serviceResponse.data.value.id; - // Create another service const anotherServiceResponse = await postServiceReq(e2eReq, { name: anotherServiceName, desc: 'Another test service for stream route isolation testing', + labels: { test: anotherServiceName }, }); anotherServiceId = anotherServiceResponse.data.value.id; - // Create test stream routes under the service for (const streamRoute of streamRoutes) { const streamRouteResponse = await postStreamRouteReq(e2eReq, { server_addr: streamRoute.server_addr, server_port: streamRoute.server_port, service_id: testServiceId, + labels: { test: serviceName }, }); + if (!streamRouteResponse.data?.value) { + throw new Error(`Failed to create stream route: ${JSON.stringify(streamRouteResponse.data)}`); + } createdStreamRoutes.push(streamRouteResponse.data.value.id); } - // Create a stream route that uses upstream directly instead of service_id - await postStreamRouteReq(e2eReq, upstreamStreamRoute); + const upstreamStreamRouteResponse = await postStreamRouteReq(e2eReq, { + ...upstreamStreamRoute, + labels: { test: 'upstream-' + serviceName }, + }); + if (!upstreamStreamRouteResponse.data?.value) { + throw new Error(`Failed to create upstream stream route: ${JSON.stringify(upstreamStreamRouteResponse.data)}`); + } + upstreamStreamRouteId = upstreamStreamRouteResponse.data.value.id; - // Create a stream route under another service - await postStreamRouteReq(e2eReq, { + const anotherServiceStreamRouteResponse = await postStreamRouteReq(e2eReq, { ...anotherServiceStreamRoute, service_id: anotherServiceId, + labels: { test: anotherServiceName }, }); + if (!anotherServiceStreamRouteResponse.data?.value) { + throw new Error(`Failed to create another service stream route: ${JSON.stringify(anotherServiceStreamRouteResponse.data)}`); + } + anotherServiceStreamRouteId = anotherServiceStreamRouteResponse.data.value.id; + + // Significant delay for backend consistency in intensive parallel CI + // Moved to end of setup to ensure all resources have time to propagate + await new Promise((resolve) => setTimeout(resolve, 8000)); Review Comment: An 8-second delay (8000ms) is added after test setup to ensure backend consistency. This significantly increases test execution time. Consider if this delay is truly necessary or if there's a more efficient way to ensure data propagation, such as polling for the resource to be available or using backend synchronization mechanisms. ########## src/routes/stream_routes/index.tsx: ########## @@ -100,37 +94,13 @@ export const StreamRouteList = (props: StreamRouteListProps) => { }, [t, ToDetailBtn, refetch]); return ( - <AntdConfigProvider> - <ProTable - columns={columns} - dataSource={data.list} - rowKey="id" - loading={isLoading} - search={false} - options={false} - pagination={pagination} - cardProps={{ bodyStyle: { padding: 0 } }} - toolbar={{ - menu: { - type: 'inline', - items: [ - { - key: 'add', - label: ( - <ToAddPageBtn - key="add" - label={t('info.add.title', { - name: t('streamRoutes.singular'), - })} - to={`${routeKey}add`} - /> - ), - }, - ], - }, - }} - /> - </AntdConfigProvider> + <ResourceListPage + columns={columns} + queryHook={() => ({ data, isLoading, pagination, refetch })} + rowKey="id" + addPageTo={`${routeKey}add`} + resourceNameKey="streamRoutes.singular" + /> Review Comment: The `titleKey` prop is not provided to ResourceListPage for stream routes, but it was not present in the original implementation either (no PageHeader was used). However, this creates inconsistency with other resource lists that do have titles (e.g., services, upstreams, etc.). Consider whether stream routes should also have a page title for consistency. ########## src/types/schema/pageSearch.ts: ########## @@ -19,17 +19,16 @@ 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() + ), + page_size: z.preprocess( + (val) => (val === undefined || val === null ? undefined : Number(val)), + z.number().optional() + ), name: z.string().optional(), + search: z.string().optional(), Review Comment: The E2E tests are using a `search` parameter (line 186 in services.stream_routes.list.spec.ts, line 192 in services.routes.list.spec.ts), which is added to pageSearchSchema.ts but not actually implemented in the UI or API integration. The tests set this parameter but there's no evidence it's being used for filtering. This could lead to false test passes if the tests rely on this filtering working. ########## src/types/schema/pageSearch.ts: ########## @@ -19,17 +19,16 @@ 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() + ), + page_size: z.preprocess( + (val) => (val === undefined || val === null ? undefined : Number(val)), + z.number().optional() + ), name: z.string().optional(), + search: z.string().optional(), Review Comment: A new `search` field has been added to the page search schema, but this field is not used in any of the refactored components. This appears to be prepared for future functionality but is not documented or utilized. Either remove this unused field or document its intended purpose. ########## e2e/utils/test.ts: ########## @@ -43,31 +48,46 @@ 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' }); + + // we need to authenticate + const settingsModal = page.getByRole('dialog', { name: 'Settings' }); + await expect(settingsModal).toBeVisible({ timeout: 30000 }); + // PasswordInput renders with a label, use getByLabel instead + const adminKeyInput = page.getByLabel('Admin Key'); + await adminKeyInput.clear(); + await adminKeyInput.fill(adminKey); + + const closeButton = page + .getByRole('dialog', { name: 'Settings' }) + .getByRole('button'); + await expect(closeButton).toBeVisible({ timeout: 10000 }); + await closeButton.click(); - // have to use env here, because the baseURL is not available in worker - await page.goto(env.E2E_TARGET_URL); + // Wait for auth to complete + await expect(settingsModal).toBeHidden({ timeout: 15000 }); - // we need to authenticate - const settingsModal = page.getByRole('dialog', { name: 'Settings' }); - await expect(settingsModal).toBeVisible(); - // PasswordInput renders with a label, use getByLabel instead - const adminKeyInput = page.getByLabel('Admin Key'); - await adminKeyInput.clear(); - await adminKeyInput.fill(adminKey); - await page - .getByRole('dialog', { name: 'Settings' }) - .getByRole('button') - .click(); + // Wait for any post-auth navigation/loading to complete + await page.waitForLoadState('load'); + + await page.context().storageState({ path: fileName }); + await page.close(); + } catch (error) { + console.error(`Failed to authenticate worker ${id}:`, error); + throw error; + } - await page.context().storageState({ path: fileName }); - await page.close(); await use(fileName); }, { scope: 'worker' }, ], page: async ({ baseURL, page }, use) => { - await page.goto(baseURL); + await page.goto(baseURL ?? ''); Review Comment: The baseURL can now be an empty string if undefined. This could cause navigation issues if baseURL is not properly configured. Consider using a fallback or validation: `await page.goto(baseURL ?? env.E2E_TARGET_URL);` to ensure a valid URL is always used. ```suggestion await page.goto(baseURL && baseURL.length > 0 ? baseURL : env.E2E_TARGET_URL); ``` ########## e2e/utils/req.ts: ########## @@ -27,27 +27,39 @@ export const getPlaywrightRequestAdapter = ( ctx: APIRequestContext ): AxiosAdapter => { return async (config) => { - const { url, data, baseURL } = config; + const { url, data, baseURL, method } = config; if (typeof url === 'undefined') { throw new Error('Need to provide a url'); } type Payload = Parameters<APIRequestContext['fetch']>[1]; const payload: Payload = { headers: config.headers, - method: config.method, - failOnStatusCode: true, + method, + failOnStatusCode: false, data, }; const urlWithBase = `${baseURL}${url}`; const res = await ctx.fetch(urlWithBase, payload); + const status = res.status(); + + // Idempotent DELETE: Treat 404 as 200 OK + if (method?.toLowerCase() === 'delete' && status === 404) { + return { + data: {}, + status: 200, + statusText: 'OK', + headers: {}, + config, + }; + } Review Comment: Good improvement for test stability! Treating DELETE 404 as success makes tests more robust to timing issues and parallel execution. However, this silent handling could mask actual issues where a resource should exist but doesn't. Consider adding a warning log when this occurs, or document this behavior clearly in tests to avoid confusion. -- 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]
