Copilot commented on code in PR #3222: URL: https://github.com/apache/apisix-dashboard/pull/3222#discussion_r2869617780
########## src/utils/clientSideFilter.ts: ########## @@ -0,0 +1,161 @@ +/** + * 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 { STATUS_ALL } from '@/config/constant'; +import type { APISIXType } from '@/types/schema/apisix'; + +import type { SearchFormValues } from '../components/form/SearchForm'; + +/** + * Client-side filtering utility for routes + * Used as a fallback when backend doesn't support certain filter parameters + */ + +export const filterRoutes = ( + routes: APISIXType['RespRouteItem'][], + filters: SearchFormValues +): APISIXType['RespRouteItem'][] => { + return routes.filter((route) => { + const routeData = route.value; + + // Filter by name + if (filters.name) { + const nameMatch = routeData.name + ?.toLowerCase() + .includes(filters.name.toLowerCase()); + if (!nameMatch) return false; + } + + // Filter by ID + if (filters.id) { + const idMatch = String(routeData.id) + .toLowerCase() + .includes(filters.id.toLowerCase()); + if (!idMatch) return false; + } + + // Filter by host + if (filters.host) { + const host = Array.isArray(routeData.host) + ? routeData.host.join(',') + : routeData.host || ''; + const hosts = Array.isArray((routeData as unknown as Record<string, string[]>).hosts) + ? (routeData as unknown as Record<string, string[]>).hosts.join(',') + : ''; + const combinedHost = `${host} ${hosts}`.toLowerCase(); + const hostMatch = combinedHost.includes(filters.host.toLowerCase()); + if (!hostMatch) return false; + } + + // Filter by path/URI + if (filters.path) { + const uri = Array.isArray(routeData.uri) + ? routeData.uri.join(',') + : routeData.uri || ''; + const uris = Array.isArray(routeData.uris) + ? routeData.uris.join(',') + : ''; + const combinedPath = `${uri} ${uris}`.toLowerCase(); + const pathMatch = combinedPath.includes(filters.path.toLowerCase()); + if (!pathMatch) return false; + } + + // Filter by description + // Note: Routes without a description field are excluded when description filter is active + if (filters.description) { + const descMatch = (routeData.desc || '') + .toLowerCase() + .includes(filters.description.toLowerCase()); + if (!descMatch) return false; + } + + // Filter by plugin: treat the filter text as a substring across all plugin names + // Note: Routes without any plugins are excluded when plugin filter is active + if (filters.plugin) { + if (!routeData.plugins) return false; + const pluginNames = Object.keys(routeData.plugins).join(',').toLowerCase(); + const pluginMatch = pluginNames.includes(filters.plugin.toLowerCase()); + if (!pluginMatch) return false; + } + + // Filter by labels: match provided label key:value tokens against route label pairs + // Note: Routes without any labels are excluded when labels filter is active + if (filters.labels && filters.labels.length > 0) { + if (!routeData.labels) return false; + + const routeLabels = Object.keys(routeData.labels).map((key) => + `${key}:${routeData.labels![key]}`.toLowerCase() + ); + const hasMatchingLabel = filters.labels.some((filterLabel) => + routeLabels.some((routeLabel) => + routeLabel.includes(filterLabel.toLowerCase()) + ) + ); + if (!hasMatchingLabel) return false; Review Comment: `filters.labels` is assumed to be a `string[]` and is used with `.length` and `.some()`, but when filters come from URL search params a single label is commonly represented as a string (not an array). In that case this will throw at runtime (`some is not a function`). Consider normalizing `filters.labels` to an array (e.g., `const labelFilters = Array.isArray(filters.labels) ? filters.labels : filters.labels ? [filters.labels] : []`) or widening the type to `string | string[]` and handling both cases consistently. ```suggestion if (filters.labels) { const labelFilters = Array.isArray(filters.labels) ? filters.labels : [filters.labels]; if (labelFilters.length > 0) { if (!routeData.labels) return false; const routeLabels = Object.keys(routeData.labels).map((key) => `${key}:${routeData.labels![key]}`.toLowerCase() ); const hasMatchingLabel = labelFilters.some((filterLabel) => routeLabels.some((routeLabel) => routeLabel.includes(filterLabel.toLowerCase()) ) ); if (!hasMatchingLabel) return false; } ``` ########## src/routes/routes/index.tsx: ########## @@ -40,14 +50,134 @@ export type RouteListProps = { }) => React.ReactNode; }; +const SEARCH_PARAM_KEYS: (keyof SearchFormValues)[] = [ + 'name', + 'id', + 'host', + 'path', + 'description', + 'plugin', + 'labels', + 'version', + 'status', +]; + +const mapSearchParams = (values: Partial<SearchFormValues>) => + Object.fromEntries( + SEARCH_PARAM_KEYS + .map((key) => [key, values[key]] as const) + .filter(([, value]) => value !== undefined) + ) as Partial<SearchFormValues>; + export const RouteList = (props: RouteListProps) => { const { routeKey, ToDetailBtn, defaultParams } = props; - const { data, isLoading, refetch, pagination } = useRouteList( + const { data, isLoading, refetch, pagination, setParams: setRouteListParams } = useRouteList( routeKey, defaultParams ); + const { params, resetParams, setParams: setSearchParams } = useSearchParams(routeKey) as { + params: PageSearchType; + resetParams: (defaults?: Partial<PageSearchType>) => void; + setParams: (params: Partial<PageSearchType>) => void; + }; const { t } = useTranslation(); + // Client-side filtering strategy: + // The backend API has limited support for complex filtering (e.g., fuzzy search on description, + // plugin content) combined with pagination. Therefore, we use client-side filtering for these fields. + // Limitation: We only fetch the first PAGE_SIZE_MAX records. If the total number of records + // exceeds this limit, the filter may miss matching records that are not in the first batch. + // This limitation is communicated to the user via a warning alert in the table. + const needsAllData = needsClientSideFiltering(params); + const nameFilter = params.name; + const { data: allData, isLoading: isLoadingAllData } = useQuery({ + queryKey: ['routes-all', defaultParams, nameFilter], + queryFn: () => + getRouteListReq(req, { + page: 1, + page_size: PAGE_SIZE_MAX, + ...defaultParams, + ...(nameFilter ? { name: nameFilter } : {}), + }), + enabled: needsAllData, + }); Review Comment: `params` comes from `validateSearch: pageSearchSchema` (which passthroughs unknown keys), but this component uses `params` as if it were `SearchFormValues` (e.g., `labels` as `string[]`, `name` as `string`). When those values originate from the URL, they may not have the expected types (notably `labels` can be a single string), which can cause runtime errors in `needsClientSideFiltering`/`filterRoutes`. Recommend either (a) validating/parsing route search params with a routes-specific zod schema before using them as filters, or (b) normalizing/coercing each filter field here before passing it to filtering utilities. ########## src/types/schema/pageSearch.ts: ########## @@ -29,8 +29,6 @@ export const pageSearchSchema = z .optional() .default(10) .transform((val) => (val ? Number(val) : 10)), Review Comment: `pageSearchSchema` no longer defines any of the new route search filter fields (and even removed `name`/`label`), but `/routes/` still uses it as `validateSearch`. This means filter params coming from the URL (e.g., `labels`) won't be parsed/normalized and can end up as unexpected shapes (string vs string[]) when reloading/sharing URLs, which will break the client-side filtering logic that assumes `SearchFormValues` types. Consider introducing a route-specific `validateSearch` schema for routes that extends `pageSearchSchema` and explicitly defines/coerces `name`, `labels`, `version`, `status`, etc. (especially coercing `labels` into `string[]`). ```suggestion .transform((val) => (val ? Number(val) : 10)), // Common search filter fields used by routes and other pages. // Keeping these optional preserves backward compatibility while // ensuring URL params are normalized into consistent shapes. name: z.string().optional(), version: z.string().optional(), labels: z .preprocess((val) => { if (val === undefined || val === null) { return []; } if (Array.isArray(val)) { return val; } return [val]; }, z.array(z.string())) .optional() .default([]), status: z .preprocess((val) => { if (val === undefined || val === null) { return []; } if (Array.isArray(val)) { return val; } return [val]; }, z.array(z.string())) .optional(), ``` ########## src/routes/routes/index.tsx: ########## @@ -40,14 +50,134 @@ export type RouteListProps = { }) => React.ReactNode; }; +const SEARCH_PARAM_KEYS: (keyof SearchFormValues)[] = [ + 'name', + 'id', + 'host', + 'path', + 'description', + 'plugin', + 'labels', + 'version', + 'status', +]; + +const mapSearchParams = (values: Partial<SearchFormValues>) => + Object.fromEntries( + SEARCH_PARAM_KEYS + .map((key) => [key, values[key]] as const) + .filter(([, value]) => value !== undefined) + ) as Partial<SearchFormValues>; + export const RouteList = (props: RouteListProps) => { const { routeKey, ToDetailBtn, defaultParams } = props; - const { data, isLoading, refetch, pagination } = useRouteList( + const { data, isLoading, refetch, pagination, setParams: setRouteListParams } = useRouteList( routeKey, defaultParams ); + const { params, resetParams, setParams: setSearchParams } = useSearchParams(routeKey) as { + params: PageSearchType; + resetParams: (defaults?: Partial<PageSearchType>) => void; + setParams: (params: Partial<PageSearchType>) => void; + }; const { t } = useTranslation(); + // Client-side filtering strategy: + // The backend API has limited support for complex filtering (e.g., fuzzy search on description, + // plugin content) combined with pagination. Therefore, we use client-side filtering for these fields. + // Limitation: We only fetch the first PAGE_SIZE_MAX records. If the total number of records + // exceeds this limit, the filter may miss matching records that are not in the first batch. + // This limitation is communicated to the user via a warning alert in the table. + const needsAllData = needsClientSideFiltering(params); + const nameFilter = params.name; + const { data: allData, isLoading: isLoadingAllData } = useQuery({ + queryKey: ['routes-all', defaultParams, nameFilter], + queryFn: () => + getRouteListReq(req, { + page: 1, + page_size: PAGE_SIZE_MAX, + ...defaultParams, + ...(nameFilter ? { name: nameFilter } : {}), + }), + enabled: needsAllData, + }); + + const handleSearch = (values: SearchFormValues) => { + // Send name filter to backend, keep others for client-side filtering + setRouteListParams({ + page: 1, + ...mapSearchParams(values), + }); + }; + + const handleReset = () => { + // Clear existing search params and then re-apply the default status filter + resetParams({ + page: 1, + status: STATUS_ALL, + }); + }; + + // Apply client-side filtering and pagination + const { filteredData, totalCount } = useMemo(() => { + // If client-side filtering is needed, use all data + if (needsAllData && allData?.list) { + const filtered = filterRoutes(allData.list, params); + const paginated = paginateResults( + filtered, + params.page || 1, + params.page_size || 10 + ); + return { + filteredData: paginated.list, + totalCount: paginated.total, + }; + } + + // Otherwise, use paginated data from backend + return { + filteredData: data?.list || [], + totalCount: data?.total || 0, + }; + }, [needsAllData, allData, data, params]); + + const actualLoading = needsAllData ? isLoadingAllData : isLoading; + + // Update pagination to use filtered total and prevent unnecessary refetch during client-side pagination + const customPagination = useMemo(() => { + return { + ...pagination, + total: totalCount, + // When client-side filtering is active, only update URL params without triggering refetch + // since all data is already loaded + onChange: needsAllData + ? (page: number, pageSize: number) => { + setSearchParams({ page, page_size: pageSize }); + } + : pagination.onChange, + }; + }, [pagination, totalCount, needsAllData, setSearchParams]); Review Comment: `customPagination.onChange` tries to avoid backend refetch when `needsAllData` is true, but updating route search params will still change `useRouteList`’s query key (`useSuspenseQuery(listQueryOptions({ ...defaultParams, ...params }))`) and trigger a backend fetch for each page change. If the intent is client-only pagination in this mode, consider keeping page/page_size in local component state (or adding an `enabled`/skip option to `genUseList`) so the server list query is disabled while client-side filtering is active. ########## src/components/form/SearchForm.tsx: ########## @@ -0,0 +1,249 @@ +/** + * 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 { AutoComplete, Button, Col, Form, Input, Row, Select, Space } from 'antd'; +import { useEffect, useMemo } from 'react'; +import { useTranslation } from 'react-i18next'; + +import { STATUS_ALL } from '@/config/constant'; + +export type SearchFormValues = { + name?: string; + id?: string; + host?: string; + path?: string; + description?: string; + plugin?: string; + labels?: string[]; + version?: string; + status?: string; +}; + +type Option = { + label: string; + value: string; +}; + +type SearchFormProps = { + onSearch?: (values: SearchFormValues) => void; + onReset?: (values: SearchFormValues) => void; + labelOptions?: Option[]; + versionOptions?: Option[]; + statusOptions?: Option[]; + initialValues?: Partial<SearchFormValues>; +}; + +export const SearchForm = (props: SearchFormProps) => { + const { + onSearch, + onReset, + labelOptions, + versionOptions, + statusOptions, + initialValues, + } = props; + + const { t } = useTranslation('common'); + const [form] = Form.useForm<SearchFormValues>(); + + const defaultStatusOptions = useMemo<Option[]>( + () => [ + { + label: t('form.searchForm.status.all'), + value: STATUS_ALL, + }, + { + label: t('form.searchForm.status.published'), + value: 'Published', + }, + { + label: t('form.searchForm.status.unpublished'), + value: 'UnPublished', + }, + ], + [t] + ); + + const mergedStatusOptions = useMemo( + () => statusOptions ?? defaultStatusOptions, + [defaultStatusOptions, statusOptions] + ); + const resolvedInitialValues = useMemo(() => { + const defaultStatus = mergedStatusOptions[0]?.value ?? undefined; + return { + status: defaultStatus, + ...initialValues, + } satisfies SearchFormValues; + }, [initialValues, mergedStatusOptions]); + + useEffect(() => { + form.setFieldsValue(resolvedInitialValues); + }, [form, resolvedInitialValues]); + + const handleFinish = (values: SearchFormValues) => { + onSearch?.(values); + }; + + const handleReset = async () => { + form.resetFields(); + form.setFieldsValue(resolvedInitialValues); + const values = form.getFieldsValue(); + if (onReset) { + onReset(values); + } else { + onSearch?.(values); + } + }; + + return ( + <Form + form={form} + layout="vertical" + autoComplete="off" + onFinish={handleFinish} + style={{ + padding: '20px', + background: '#fafafa', + borderRadius: '8px', + border: '1px solid #e8e8e8', + }} + > + <Row gutter={[16, 0]}> + {/* First column - spans 2 rows */} + <Col xs={24} sm={12} md={6}> + <Form.Item<SearchFormValues> + name="name" + label={t('form.searchForm.fields.name')} + style={{ marginBottom: '16px' }} + > + <Input placeholder={t('form.searchForm.placeholders.name')} allowClear /> + </Form.Item> + <Form.Item<SearchFormValues> + name="id" + label={t('form.searchForm.fields.id')} + style={{ marginBottom: '16px' }} + > + <Input placeholder={t('form.searchForm.placeholders.id')} allowClear /> + </Form.Item> + </Col> + + {/* Second column - first row */} + <Col xs={24} sm={12} md={6}> + <Form.Item<SearchFormValues> + name="host" + label={t('form.searchForm.fields.host')} + style={{ marginBottom: '16px' }} + > + <Input placeholder={t('form.searchForm.placeholders.host')} allowClear /> + </Form.Item> + {/* Second column - second row */} + <Form.Item<SearchFormValues> + name="plugin" + label={t('form.searchForm.fields.plugin')} + style={{ marginBottom: '16px' }} + > + <Input placeholder={t('form.searchForm.placeholders.plugin')} allowClear /> + </Form.Item> + </Col> + + {/* Third column - first row */} + <Col xs={24} sm={12} md={6}> + <Form.Item<SearchFormValues> + name="path" + label={t('form.searchForm.fields.path')} + style={{ marginBottom: '16px' }} + > + <Input placeholder={t('form.searchForm.placeholders.path')} allowClear /> + </Form.Item> + {/* Third column - second row */} + <Form.Item<SearchFormValues> + name="labels" + label={t('form.searchForm.fields.labels')} + style={{ marginBottom: '16px' }} + > + <Select + mode="tags" + placeholder={t('form.searchForm.placeholders.labels')} + allowClear + options={labelOptions} + /> + </Form.Item> + </Col> + + {/* Fourth column - first row */} + <Col xs={24} sm={12} md={6}> + <Form.Item<SearchFormValues> + name="description" + label={t('form.searchForm.fields.description')} + style={{ marginBottom: '16px' }} + > + <Input placeholder={t('form.searchForm.placeholders.description')} allowClear /> + </Form.Item> + {/* Fourth column - second row */} + <Form.Item<SearchFormValues> + name="version" + label={t('form.searchForm.fields.version')} + style={{ marginBottom: '16px' }} + > + <AutoComplete + placeholder={t('form.searchForm.placeholders.version')} + allowClear + options={versionOptions} + /> + </Form.Item> + </Col> + </Row> + + {/* Third row - Status and Actions */} + <Row gutter={[16, 0]} align="bottom" style={{ marginTop: '8px' }}> + <Col xs={24} sm={12} md={6}> + <Form.Item<SearchFormValues> + name="status" + label={t('form.searchForm.fields.status')} + style={{ marginBottom: 0 }} + > + <Select + placeholder={t('form.searchForm.placeholders.status')} + allowClear + options={mergedStatusOptions} + /> + </Form.Item> + </Col> + <Col xs={24} sm={12} md={18}> + <div + style={{ + display: 'flex', + justifyContent: 'flex-end', + alignItems: 'center', + height: '32px', + }} + > + <Space size="middle"> + <Button onClick={handleReset} size="middle"> + {t('form.searchForm.actions.reset')} + </Button> + <Button type="primary" htmlType="submit" size="middle"> + {t('form.searchForm.actions.search')} + </Button> + </Space> + </div> + </Col> + </Row> + </Form > Review Comment: The closing tag `</Form >` has an extra space before `>`, which may violate JSX lint rules and is inconsistent with the rest of the codebase’s JSX formatting. Consider changing it to `</Form>`. ```suggestion </Form> ``` -- 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]
