Copilot commented on code in PR #3222:
URL: https://github.com/apache/apisix-dashboard/pull/3222#discussion_r2828190434
##########
src/routes/routes/index.tsx:
##########
@@ -95,14 +221,23 @@ export const RouteList = (props: RouteListProps) => {
return (
<AntdConfigProvider>
+ <div style={{ marginBottom: 24 }}>
+ <SearchForm
+ onSearch={handleSearch}
+ onReset={handleReset}
+ versionOptions={versionOptions}
+ labelOptions={versionOptions}
Review Comment:
The `labelOptions` prop is incorrectly set to `versionOptions`. The labels
field is intended for filtering by arbitrary route labels (key:value pairs),
not just version labels. This should either be a separate set of options
extracted from all route labels, or the field should accept free-form text
input instead of a select dropdown with predefined options.
```suggestion
```
##########
src/locales/de/common.json:
##########
@@ -87,8 +87,42 @@
"uris": "URIs",
"vars": "Variablen"
},
- "search": "Suche",
- "secrets": {
+ "search": "Suche",
+ "searchForm": {
+ "fields": {
+ "name": "Name",
+ "id": "ID",
+ "host": "Host",
+ "path": "Pfad",
+ "description": "Beschreibung",
+ "plugin": "Plugin",
+ "labels": "Labels",
+ "version": "Version",
+ "status": "Status"
+ },
+ "placeholders": {
+ "name": "Name",
+ "id": "ID",
+ "host": "Host",
+ "path": "Pfad",
+ "description": "Beschreibung",
+ "plugin": "Plugin",
+ "labels": "Labels auswählen",
+ "version": "Version auswählen",
+ "status": "Status auswählen"
+ },
+ "status": {
+ "all": "Unveröffentlicht/Veröffentlicht",
+ "published": "Veröffentlicht",
+ "unpublished": "Unveröffentlicht"
+ },
+ "actions": {
+ "reset": "Zurücksetzen",
+ "search": "Suchen"
+ }
+ },
+ "searchLimit": "Es werden nur die ersten {{count}} Routen aus der
Datenbank abgerufen. Die clientseitige Filterung wird auf diese Datensätze
angewendet.",
+ "secrets": {
Review Comment:
Inconsistent indentation for the new search form translations. The `search`,
`searchForm`, and `searchLimit` entries use excessive indentation (12 spaces)
compared to the rest of the file which uses 4 spaces. This should be corrected
to maintain consistent JSON formatting throughout the file.
```suggestion
"search": "Suche",
"searchForm": {
"fields": {
"name": "Name",
"id": "ID",
"host": "Host",
"path": "Pfad",
"description": "Beschreibung",
"plugin": "Plugin",
"labels": "Labels",
"version": "Version",
"status": "Status"
},
"placeholders": {
"name": "Name",
"id": "ID",
"host": "Host",
"path": "Pfad",
"description": "Beschreibung",
"plugin": "Plugin",
"labels": "Labels auswählen",
"version": "Version auswählen",
"status": "Status auswählen"
},
"status": {
"all": "Unveröffentlicht/Veröffentlicht",
"published": "Veröffentlicht",
"unpublished": "Unveröffentlicht"
},
"actions": {
"reset": "Zurücksetzen",
"search": "Suchen"
}
},
"searchLimit": "Es werden nur die ersten {{count}} Routen aus der
Datenbank abgerufen. Die clientseitige Filterung wird auf diese Datensätze
angewendet.",
"secrets": {
```
##########
src/routes/routes/index.tsx:
##########
@@ -40,14 +50,130 @@ 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 } = useRouteList(
routeKey,
defaultParams
);
+ const { params, resetParams, setParams: setSearchParams } =
useSearchParams(routeKey) as {
+ params: PageSearchType;
+ resetParams: () => void;
+ setParams: (params: Partial<PageSearchType>) => void;
+ };
const { t } = useTranslation();
+ // Fetch all data when client-side filtering is active
+ 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
+ setParams({
+ page: 1,
+ ...mapSearchParams(values),
+ });
+ };
+
+ const handleReset = () => {
+ // Clear existing search params and then re-apply the default status filter
+ resetParams();
+ setParams({
+ page: 1,
+ status: STATUS_ALL,
+ });
+ };
Review Comment:
The `handleReset` function makes two separate navigation calls - first
`resetParams()` to clear all search params, then `setParams()` to set the
status. This could cause race conditions or unnecessary re-renders. Consider
combining into a single navigation call by using `setParams({ page: 1, status:
STATUS_ALL })` directly without calling `resetParams()` first, or modify
`resetParams` to accept default values to set after clearing.
##########
src/utils/clientSideFilter.ts:
##########
@@ -0,0 +1,155 @@
+/**
+ * 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 hostMatch =
host.toLowerCase().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 && routeData.desc) {
+ 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 && routeData.plugins) {
+ 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;
+ }
+
+ // Filter by version
+ if (filters.version && routeData.labels?.version) {
Review Comment:
The version filter incorrectly includes routes without a version label when
the filter is active. The condition `if (filters.version &&
routeData.labels?.version)` only filters routes that have both a filter value
AND a version label. Routes without a `labels.version` field will pass through
the filter, which is incorrect behavior. When a version filter is applied,
routes without version labels should be excluded. Change to:
```
if (filters.version) {
if (!routeData.labels?.version) return false;
const versionMatch = routeData.labels.version === filters.version;
if (!versionMatch) return false;
}
```
```suggestion
if (filters.version) {
if (!routeData.labels?.version) return false;
```
##########
src/locales/en/common.json:
##########
@@ -88,6 +88,39 @@
"vars": "Vars"
},
"search": "Search",
+ "searchForm": {
+ "fields": {
+ "name": "Name",
+ "id": "ID",
+ "host": "Host",
+ "path": "Path",
+ "description": "Description",
+ "plugin": "Plugin",
+ "labels": "Labels",
+ "version": "Version",
+ "status": "Status"
+ },
+ "placeholders": {
+ "name": "Name",
+ "id": "ID",
+ "host": "Host",
+ "path": "Path",
+ "description": "Description",
+ "plugin": "Plugin",
+ "labels": "Select labels",
+ "version": "Select version",
+ "status": "Select status"
+ },
+ "status": {
+ "all": "UnPublished/Published",
+ "published": "Published",
+ "unpublished": "UnPublished"
+ },
+ "actions": {
+ "reset": "Reset",
+ "search": "Search"
+ }
+ },
Review Comment:
The PR description claims "Added translations for 4 locales (en, es, de,
zh)" but the Chinese (zh) and Turkish (tr) locale files are missing the new
`searchForm` and `searchLimit` translations. These translations should be added
to maintain consistency across all supported locales.
##########
src/locales/es/common.json:
##########
@@ -87,7 +87,41 @@
"uris": "URIs",
"vars": "Variables"
},
- "search": "Buscar",
+ "search": "Buscar",
+ "searchLimit": "Solo se recuperan las primeras {{count}} rutas de
la base de datos. El filtrado del lado del cliente se aplica a estos
registros.",
+ "searchForm": {
+ "fields": {
+ "name": "Nombre",
+ "id": "ID",
+ "host": "Host",
+ "path": "Ruta",
+ "description": "Descripción",
+ "plugin": "Plugin",
+ "labels": "Etiquetas",
+ "version": "Versión",
+ "status": "Estado"
+ },
+ "placeholders": {
+ "name": "Nombre",
+ "id": "ID",
+ "host": "Host",
+ "path": "Ruta",
+ "description": "Descripción",
+ "plugin": "Plugin",
+ "labels": "Selecciona etiquetas",
+ "version": "Selecciona versión",
+ "status": "Selecciona estado"
+ },
+ "status": {
+ "all": "Sin publicar/Publicado",
+ "published": "Publicado",
+ "unpublished": "Sin publicar"
+ },
+ "actions": {
+ "reset": "Restablecer",
+ "search": "Buscar"
+ }
+ },
Review Comment:
Inconsistent indentation for the new search form translations. The
`searchForm` and `searchLimit` entries use excessive indentation (12 spaces)
compared to the rest of the file which uses 4 spaces. This should be corrected
to maintain consistent JSON formatting throughout the file.
```suggestion
"search": "Buscar",
"searchLimit": "Solo se recuperan las primeras {{count}} rutas de la
base de datos. El filtrado del lado del cliente se aplica a estos registros.",
"searchForm": {
"fields": {
"name": "Nombre",
"id": "ID",
"host": "Host",
"path": "Ruta",
"description": "Descripción",
"plugin": "Plugin",
"labels": "Etiquetas",
"version": "Versión",
"status": "Estado"
},
"placeholders": {
"name": "Nombre",
"id": "ID",
"host": "Host",
"path": "Ruta",
"description": "Descripción",
"plugin": "Plugin",
"labels": "Selecciona etiquetas",
"version": "Selecciona versión",
"status": "Selecciona estado"
},
"status": {
"all": "Sin publicar/Publicado",
"published": "Publicado",
"unpublished": "Sin publicar"
},
"actions": {
"reset": "Restablecer",
"search": "Buscar"
}
},
```
##########
src/routes/routes/index.tsx:
##########
@@ -40,14 +50,130 @@ 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 } = useRouteList(
routeKey,
defaultParams
);
+ const { params, resetParams, setParams: setSearchParams } =
useSearchParams(routeKey) as {
+ params: PageSearchType;
+ resetParams: () => void;
+ setParams: (params: Partial<PageSearchType>) => void;
+ };
const { t } = useTranslation();
+ // Fetch all data when client-side filtering is active
+ 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:
When client-side filtering is active, the implementation fetches up to 500
routes (PAGE_SIZE_MAX) and filters them in the browser. This could cause
performance issues if a cluster has more than 500 routes, as the warning
message only informs users about the limitation but doesn't prevent confusion
when results are incomplete. Consider implementing server-side pagination for
filtered results if the backend API supports it, or at least document this
limitation more prominently in the component or user documentation.
##########
src/utils/clientSideFilter.ts:
##########
@@ -0,0 +1,155 @@
+/**
+ * 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 hostMatch =
host.toLowerCase().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 && routeData.desc) {
+ 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 && routeData.plugins) {
+ const pluginNames =
Object.keys(routeData.plugins).join(',').toLowerCase();
+ const pluginMatch = pluginNames.includes(filters.plugin.toLowerCase());
+ if (!pluginMatch) return false;
+ }
Review Comment:
The plugin filter incorrectly includes routes without plugins when the
filter is active. The condition `if (filters.plugin && routeData.plugins)` only
filters routes that have both a filter value AND plugins. Routes without any
plugins will pass through the filter, which is incorrect behavior. When a
plugin filter is applied, routes without plugins should be excluded. Change to:
```
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;
}
```
--
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]
