Copilot commented on code in PR #3222:
URL: https://github.com/apache/apisix-dashboard/pull/3222#discussion_r2802132626
##########
src/routes/routes/index.tsx:
##########
@@ -95,14 +214,22 @@ export const RouteList = (props: RouteListProps) => {
return (
<AntdConfigProvider>
+ <div style={{ marginBottom: 24 }}>
+ <SearchForm
+ onSearch={handleSearch}
+ onReset={handleReset}
+ versionOptions={versionOptions}
+ initialValues={mapSearchParams(params)}
+ />
+ </div>
Review Comment:
The versionOptions are extracted dynamically from route labels, but the
SearchForm doesn't display labelOptions. Looking at the SearchForm component,
it accepts labelOptions as a prop but never receives them from the RouteList
component. This means the labels select field will be empty and users won't be
able to select any labels to filter by. Either remove the labels field from the
form or populate labelOptions similar to how versionOptions is populated.
```suggestion
labelOptions={versionOptions}
initialValues={mapSearchParams(params)}
/>
```
##########
src/routes/routes/index.tsx:
##########
@@ -40,14 +50,123 @@ export type RouteListProps = {
}) => React.ReactNode;
};
+const RouteDetailButton = ({
+ record,
+}: {
+ record: APISIXType['RespRouteItem'];
+}) => (
+ <ToDetailPageBtn
+ key="detail"
+ to="/routes/detail/$id"
+ params={{ id: record.value.id }}
+ />
+);
+
+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
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 } = useSearchParams(routeKey) as {
+ params: PageSearchType;
+ resetParams: () => void;
+ };
const { t } = useTranslation();
+ // Fetch all data when client-side filtering is active
+ const needsAllData = needsClientSideFiltering(params);
+ const { data: allData, isLoading: isLoadingAllData } = useQuery({
+ queryKey: ['routes-all', defaultParams, needsAllData],
+ queryFn: () =>
+ getRouteListReq(req, {
+ page: 1,
+ page_size: PAGE_SIZE_MAX,
+ ...defaultParams,
+ }),
+ enabled: needsAllData,
+ });
+
+ const handleSearch = (values: SearchFormValues) => {
+ // Send name filter to backend, keep others for client-side filtering
+ setParams({
+ page: 1,
+ ...mapSearchParams(values),
+ });
+ };
+
+ const handleReset = () => {
+ resetParams();
+ };
+
+ // 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
+ const customPagination = useMemo(() => {
+ return {
+ ...pagination,
+ total: totalCount,
+ };
+ }, [pagination, totalCount]);
Review Comment:
Potential performance issue: when client-side filtering is active
(needsAllData is true), pagination changes still trigger the onChange callback
from useTablePagination, which calls setParams and potentially refetch().
However, since all data is already fetched, the refetch is unnecessary and
could cause performance issues. The customPagination should override the
onChange callback to only update URL params without triggering a refetch when
needsAllData is true.
##########
src/routes/routes/index.tsx:
##########
@@ -40,14 +50,123 @@ export type RouteListProps = {
}) => React.ReactNode;
};
+const RouteDetailButton = ({
+ record,
+}: {
+ record: APISIXType['RespRouteItem'];
+}) => (
+ <ToDetailPageBtn
+ key="detail"
+ to="/routes/detail/$id"
+ params={{ id: record.value.id }}
+ />
+);
+
+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
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 } = useSearchParams(routeKey) as {
+ params: PageSearchType;
+ resetParams: () => void;
+ };
const { t } = useTranslation();
+ // Fetch all data when client-side filtering is active
+ const needsAllData = needsClientSideFiltering(params);
+ const { data: allData, isLoading: isLoadingAllData } = useQuery({
+ queryKey: ['routes-all', defaultParams, needsAllData],
+ queryFn: () =>
+ getRouteListReq(req, {
+ page: 1,
+ page_size: PAGE_SIZE_MAX,
+ ...defaultParams,
+ }),
+ enabled: needsAllData,
Review Comment:
The query key for fetching all data includes `needsAllData` as a dependency:
`['routes-all', defaultParams, needsAllData]`. However, when needsAllData
changes from true to false (e.g., when clearing all filters), the query will be
re-fetched with enabled: false, which is unnecessary. The needsAllData boolean
should not be part of the query key since it's already handled by the enabled
prop. Consider removing it from the queryKey array to avoid unnecessary cache
entries.
##########
src/utils/clientSideFilter.ts:
##########
@@ -0,0 +1,150 @@
+/**
+ * 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 { 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 && routeData.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
+ 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
+ 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
+ if (filters.labels && filters.labels.length > 0 && routeData.labels) {
+ 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) {
+ const versionMatch = routeData.labels.version === filters.version;
+ if (!versionMatch) return false;
+ }
+
+ // Filter by status
+ if (filters.status && filters.status !== 'UnPublished/Published') {
+ const isPublished = routeData.status === 1;
+ if (filters.status === 'Published' && !isPublished) return false;
+ if (filters.status === 'UnPublished' && isPublished) return false;
+ }
+
+ return true;
+ });
+};
+
+/**
+ * Check if client-side filtering is needed
+ * Returns true if any filter parameters are present
+ */
+export const needsClientSideFiltering = (
+ filters: SearchFormValues
+): boolean => {
+ return Boolean(
+ filters.name ||
+ filters.id ||
+ filters.host ||
+ filters.path ||
+ filters.description ||
+ filters.plugin ||
+ (filters.labels && filters.labels.length > 0) ||
+ filters.version ||
+ (filters.status && filters.status !== 'UnPublished/Published')
+ );
Review Comment:
The `needsClientSideFiltering` function includes `filters.name` in its
check, which causes the application to fetch all 500 records even when only the
name filter is active. Since the backend already supports name filtering via
the API, the name filter should be excluded from this check to avoid
unnecessary data fetching. This would improve performance when users search by
name only.
##########
src/routes/routes/index.tsx:
##########
@@ -40,14 +50,123 @@ export type RouteListProps = {
}) => React.ReactNode;
};
+const RouteDetailButton = ({
+ record,
+}: {
+ record: APISIXType['RespRouteItem'];
+}) => (
+ <ToDetailPageBtn
+ key="detail"
+ to="/routes/detail/$id"
+ params={{ id: record.value.id }}
+ />
+);
+
+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
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 } = useSearchParams(routeKey) as {
+ params: PageSearchType;
+ resetParams: () => void;
+ };
const { t } = useTranslation();
+ // Fetch all data when client-side filtering is active
+ const needsAllData = needsClientSideFiltering(params);
+ const { data: allData, isLoading: isLoadingAllData } = useQuery({
+ queryKey: ['routes-all', defaultParams, needsAllData],
+ queryFn: () =>
+ getRouteListReq(req, {
+ page: 1,
+ page_size: PAGE_SIZE_MAX,
+ ...defaultParams,
+ }),
+ enabled: needsAllData,
+ });
+
+ const handleSearch = (values: SearchFormValues) => {
+ // Send name filter to backend, keep others for client-side filtering
+ setParams({
+ page: 1,
+ ...mapSearchParams(values),
+ });
+ };
+
+ const handleReset = () => {
+ resetParams();
Review Comment:
The handleReset function in the routes page only calls resetParams(), which
clears all URL search parameters. However, the SearchForm's handleReset also
sets the form back to resolvedInitialValues, which includes a default status
value ('UnPublished/Published'). This mismatch could cause a state
inconsistency where the form displays a default status value, but the URL
parameters are completely empty, leading to potential filtering issues.
Consider either passing the default status in the reset or ensuring the form
and URL parameters stay in sync.
```suggestion
const handleReset = () => {
// Clear existing search params and then re-apply the default status
filter
resetParams();
setParams({
page: 1,
status: 'UnPublished/Published',
});
```
##########
src/locales/de/common.json:
##########
@@ -88,6 +88,39 @@
"vars": "Variablen"
},
"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"
+ }
+ },
"secrets": {
Review Comment:
Missing translations for the "searchLimit" message in German (de) and
Spanish (es) locales. The English locale adds this translation at line 391, but
the corresponding translations are not present in the de and es locale files.
This will cause the warning message to fall back to the English default value
when using German or Spanish.
```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": "Die Anzahl der Suchergebnisse ist begrenzt.
Bitte verfeinern Sie Ihre Suche.",
"secrets": {
```
##########
src/utils/clientSideFilter.ts:
##########
@@ -0,0 +1,150 @@
+/**
+ * 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 { 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 && routeData.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
+ 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
+ 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
+ if (filters.labels && filters.labels.length > 0 && routeData.labels) {
+ 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:
Inconsistent filtering behavior: routes without a description field will be
excluded when searching by description (due to the `routeData.desc` check), but
routes without plugins will be excluded when searching by plugin name (due to
the `routeData.plugins` check). However, routes without labels are also
excluded (due to the `routeData.labels` check). This is logically consistent,
but worth documenting. Consider adding comments to clarify that routes without
the searched field are intentionally excluded from results.
##########
src/types/schema/pageSearch.ts:
##########
@@ -31,6 +31,10 @@ export const pageSearchSchema = z
.transform((val) => (val ? Number(val) : 10)),
name: z.string().optional(),
label: z.string().optional(),
+ id: z.string().optional(),
+ host: z.string().optional(),
+ path: z.string().optional(),
+ description: z.string().optional(),
Review Comment:
The pageSearchSchema only includes string-based filter fields (id, host,
path, description) but doesn't validate the other search parameters (plugin,
labels, version, status). Since the schema uses `.passthrough()`, these
parameters will pass through without validation. However, this means URL
parameters like `labels` (which should be an array) or `status` (which should
match specific enum values) won't be validated or transformed. Consider adding
proper schema validation for these fields to ensure type safety and prevent
invalid URL parameters.
```suggestion
description: z.string().optional(),
plugin: z.string().optional(),
labels: z.array(z.string()).optional(),
version: z.string().optional(),
status: z.string().optional(),
```
##########
src/utils/clientSideFilter.ts:
##########
@@ -0,0 +1,150 @@
+/**
+ * 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 { 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 && routeData.name) {
+ const nameMatch = routeData.name
+ .toLowerCase()
+ .includes(filters.name.toLowerCase());
+ if (!nameMatch) return false;
+ }
+
Review Comment:
The filter logic for name in clientSideFilter is redundant since name
filtering is already handled by the backend (as mentioned in the PR
description). When `needsClientSideFiltering` is true and includes a name
filter, the code fetches all data and then filters by name again on the client
side, even though the name was already sent to the backend. Consider removing
the name filter from clientSideFilter since it's handled by the backend API.
```suggestion
```
##########
src/routes/routes/index.tsx:
##########
@@ -40,14 +50,123 @@ export type RouteListProps = {
}) => React.ReactNode;
};
+const RouteDetailButton = ({
+ record,
+}: {
+ record: APISIXType['RespRouteItem'];
+}) => (
+ <ToDetailPageBtn
+ key="detail"
+ to="/routes/detail/$id"
+ params={{ id: record.value.id }}
+ />
+);
Review Comment:
The RouteDetailButton component is extracted but only used once. While this
extraction improves code readability by reducing nesting in the JSX, it doesn't
provide significant reusability benefits since it's only used in one place.
This is a minor style preference issue and not a functional problem, but
consider whether the extraction adds enough value to justify the additional
component definition.
##########
e2e/tests/routes.filter-version.spec.ts:
##########
@@ -0,0 +1,146 @@
+/**
+ * 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 { routesPom } from '@e2e/pom/routes';
+import { e2eReq } from '@e2e/utils/req';
+import { test } from '@e2e/utils/test';
+import { uiHasToastMsg } from '@e2e/utils/ui';
+import { uiFillUpstreamRequiredFields } from '@e2e/utils/ui/upstreams';
+import { expect } from '@playwright/test';
+
+import { deleteAllRoutes } from '@/apis/routes';
+import type { APISIXType } from '@/types/schema/apisix';
+
+/**
+ * Test for version filtering across multiple pages.
+ * This verifies that client-side filtering works across all routes,
+ * not just the current page.
+ */
+test.describe('Routes version filter', () => {
+ test.describe.configure({ mode: 'serial' });
+
+ const nodes: APISIXType['UpstreamNode'][] = [
+ { host: 'test.com', port: 80, weight: 100 },
+ { host: 'test2.com', port: 80, weight: 100 },
+ ];
+
+ test.beforeAll(async () => {
+ // Clean up any existing routes
+ await deleteAllRoutes(e2eReq);
+ });
+
+ test.afterAll(async () => {
+ // Clean up test routes
+ await deleteAllRoutes(e2eReq);
+ });
+
+ test('should filter routes by version across all pages', async ({ page }) =>
{
+ test.slow(); // This test creates multiple routes via UI
+
+ await test.step('create routes with different versions via UI', async ()
=> {
+ await routesPom.toIndex(page);
+ await routesPom.isIndexPage(page);
+
+ // Create 3 routes with version v1
+ for (let i = 1; i <= 3; i++) {
+ await routesPom.getAddRouteBtn(page).click();
+ await routesPom.isAddPage(page);
+
+ await page.getByLabel('Name', { exact: true
}).first().fill(`route_v1_${i}`);
+ await page.getByLabel('URI', { exact: true }).fill(`/v1/test${i}`);
+
+ // Select HTTP method
+ await page.getByRole('textbox', { name: 'HTTP Methods' }).click();
+ await page.getByRole('option', { name: 'GET' }).click();
+
+ // Add label for version (in the route section, not upstream)
+ const routeLabelsField = page.getByRole('textbox', { name: 'Labels'
}).first();
+ await routeLabelsField.click();
+ await routeLabelsField.fill('version:v1');
+ await routeLabelsField.press('Enter');
+
+ // Add upstream
+ const upstreamSection = page.getByRole('group', {
+ name: 'Upstream',
+ exact: true,
+ });
+ await uiFillUpstreamRequiredFields(upstreamSection, {
+ nodes,
+ name: `upstream_v1_${i}`,
+ desc: 'test',
+ });
+
+ await routesPom.getAddBtn(page).click();
+ await uiHasToastMsg(page, {
+ hasText: 'Add Route Successfully',
+ });
+
+ // Go back to list
+ await routesPom.getRouteNavBtn(page).click();
+ await routesPom.isIndexPage(page);
+ }
+
+ // Create 3 routes with version v2
+ for (let i = 1; i <= 3; i++) {
+ await routesPom.getAddRouteBtn(page).click();
+ await routesPom.isAddPage(page);
+
+ await page.getByLabel('Name', { exact: true
}).first().fill(`route_v2_${i}`);
+ await page.getByLabel('URI', { exact: true }).fill(`/v2/test${i}`);
+
+ // Select HTTP method
+ await page.getByRole('textbox', { name: 'HTTP Methods' }).click();
+ await page.getByRole('option', { name: 'GET' }).click();
+
+ // Add label for version (in the route section, not upstream)
+ const routeLabelsField = page.getByRole('textbox', { name: 'Labels'
}).first();
+ await routeLabelsField.click();
+ await routeLabelsField.fill('version:v2');
+ await routeLabelsField.press('Enter');
+
+ // Add upstream
+ const upstreamSection = page.getByRole('group', {
+ name: 'Upstream',
+ exact: true,
+ });
+ await uiFillUpstreamRequiredFields(upstreamSection, {
+ nodes,
+ name: `upstream_v2_${i}`,
+ desc: 'test',
+ });
+
+ await routesPom.getAddBtn(page).click();
+ await uiHasToastMsg(page, {
+ hasText: 'Add Route Successfully',
+ });
+
+ // Go back to list
+ await routesPom.getRouteNavBtn(page).click();
+ await routesPom.isIndexPage(page);
+ }
+ });
+
+ await test.step('verify all routes are created with labels', async () => {
+ // Verify routes are visible in list
+ await expect(page.getByText('route_v1_1')).toBeVisible();
+ await expect(page.getByText('route_v2_1')).toBeVisible();
+
+ // Verify version filter field exists
+ await expect(page.locator('#version')).toBeAttached();
+ });
Review Comment:
The test "should filter routes by version across all pages" doesn't actually
verify that the version filtering functionality works. It only checks that
routes are created and the version filter field exists. The test should include
steps to actually select a version from the dropdown and verify that only
routes with that version are displayed in the filtered results.
```suggestion
// Verify version filter field exists
const versionFilter = page.locator('#version');
await expect(versionFilter).toBeAttached();
});
await test.step('filter by version v1 and verify only v1 routes are
shown', async () => {
const versionFilter = page.locator('#version');
// Apply version:v1 filter
await versionFilter.click();
await versionFilter.fill('version:v1');
await versionFilter.press('Enter');
// v1 routes should be present
await expect(page.getByText('route_v1_1')).toBeVisible();
await expect(page.getByText('route_v1_2')).toBeVisible();
await expect(page.getByText('route_v1_3')).toBeVisible();
// v2 routes should not be present in the filtered results
await expect(page.getByText(/route_v2_/)).toHaveCount(0);
});
await test.step('filter by version v2 and verify only v2 routes are
shown', async () => {
// Clear previous filter by reloading the page to reset state
await page.reload();
await routesPom.isIndexPage(page);
const versionFilter = page.locator('#version');
await expect(versionFilter).toBeAttached();
// Apply version:v2 filter
await versionFilter.click();
await versionFilter.fill('version:v2');
await versionFilter.press('Enter');
// v2 routes should be present
await expect(page.getByText('route_v2_1')).toBeVisible();
await expect(page.getByText('route_v2_2')).toBeVisible();
await expect(page.getByText('route_v2_3')).toBeVisible();
// v1 routes should not be present in the filtered results
await expect(page.getByText(/route_v1_/)).toHaveCount(0);
});
```
##########
src/routes/routes/index.tsx:
##########
@@ -123,6 +250,18 @@ export const RouteList = (props: RouteListProps) => {
],
},
}}
+ tableAlertRender={
+ needsAllData
+ ? () => (
+ <span style={{ color: '#faad14' }}>
+ {t('table.searchLimit', {
+ defaultValue: `Search only allows searching in the first
${PAGE_SIZE_MAX} records.`,
+ count: PAGE_SIZE_MAX,
+ })}
+ </span>
+ )
+ : undefined
Review Comment:
The tableAlertRender is only displayed when `needsAllData` is true, but the
warning message about searching only the first 500 records could be misleading.
The search actually works across all fetched data (which is limited to 500
records via PAGE_SIZE_MAX). Consider clarifying the message to indicate that
only the first 500 routes from the database are fetched and searchable, not
that search is limited after fetching.
##########
src/utils/clientSideFilter.ts:
##########
@@ -0,0 +1,150 @@
+/**
+ * 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 { 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 && routeData.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
+ 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
+ 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
+ if (filters.labels && filters.labels.length > 0 && routeData.labels) {
+ 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:
The labels filter logic has a potential issue. When a route doesn't have
labels (routeData.labels is undefined), and the user provides a labels filter,
the route is not filtered out because the condition only checks
`routeData.labels` at the end. This means routes without labels will be
included in the results when searching for specific labels. Consider explicitly
returning false when labels filter is provided but the route has no labels.
##########
e2e/tests/routes.search.spec.ts:
##########
@@ -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 { routesPom } from '@e2e/pom/routes';
+import { e2eReq } from '@e2e/utils/req';
+import { test } from '@e2e/utils/test';
+import { expect} from '@playwright/test';
Review Comment:
Spacing issue after 'expect' import. There's an extra space before the
closing brace in the import statement.
```suggestion
import { expect } from '@playwright/test';
```
##########
src/components/form/SearchForm.tsx:
##########
@@ -0,0 +1,247 @@
+/**
+ * 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 { Button, Col, Form, Input, Row, Select, Space } from 'antd';
+import { useEffect, useMemo } from 'react';
+import { useTranslation } from 'react-i18next';
+
+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: 'UnPublished/Published',
+ },
Review Comment:
The status value 'UnPublished/Published' is used as a magic string in
multiple locations (SearchForm.tsx line 64 and clientSideFilter.ts lines 106,
132). Consider extracting this to a named constant to ensure consistency and
make it easier to maintain. For example, define `const STATUS_ALL =
'UnPublished/Published'` in a shared constants file.
##########
src/locales/es/common.json:
##########
@@ -88,6 +88,39 @@
"vars": "Variables"
},
"search": "Buscar",
+ "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:
Missing translations for the "searchLimit" message in Spanish (es) locale.
The English locale adds this translation at line 391, but the corresponding
translation is not present in the es locale file. This will cause the warning
message to fall back to the English default value when using Spanish.
```suggestion
"search": "Buscar",
"searchLimit": "La búsqueda está limitada a los primeros
{{limit}} resultados. Por favor, refine sus criterios.",
"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,123 @@ export type RouteListProps = {
}) => React.ReactNode;
};
+const RouteDetailButton = ({
+ record,
+}: {
+ record: APISIXType['RespRouteItem'];
+}) => (
+ <ToDetailPageBtn
+ key="detail"
+ to="/routes/detail/$id"
+ params={{ id: record.value.id }}
+ />
+);
+
+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
Partial<SearchFormValues>;
Review Comment:
The mapSearchParams function creates a partial SearchFormValues object but
doesn't filter out undefined values. This means that when URL parameters are
mapped back to the form, undefined values for all 9 filter fields will be
included in the object. While this may not cause functional issues, it could
lead to unnecessary entries in the params object. Consider filtering out
undefined values using Object.entries and filter before creating the returned
object.
```suggestion
Object.fromEntries(
SEARCH_PARAM_KEYS
.map((key) => [key, values[key]] as const)
.filter(([, value]) => value !== undefined)
) as Partial<SearchFormValues>;
```
##########
src/locales/en/common.json:
##########
@@ -88,6 +88,33 @@
"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": {
+ "labels": "Select labels",
+ "version": "Select version",
+ "status": "Select status"
+ },
Review Comment:
Inconsistent placeholder handling in the English locale. For text input
fields (name, id, host, path, description, plugin), the English locale doesn't
define placeholders in the "placeholders" section, while the Spanish and German
locales do. This creates an inconsistency where Spanish and German users will
see placeholder text in these fields, but English users won't. Consider either
adding placeholders for English or removing them from other locales for
consistency.
--
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]
