Copilot commented on code in PR #3295:
URL: https://github.com/apache/apisix-dashboard/pull/3295#discussion_r2954492784
##########
src/routes/routes/detail.$id.tsx:
##########
@@ -59,26 +59,30 @@ const RouteDetailForm = (props: Props) => {
const { readOnly, setReadOnly, id } = props;
const { t } = useTranslation();
- const routeQuery = useQuery(getRouteQueryOptions(id));
- const { data: routeData, isLoading, refetch } = routeQuery;
+ const routeQuery = useSuspenseQuery(getRouteQueryOptions(id));
+ const { data: routeData, refetch } = routeQuery;
Review Comment:
This change removes the explicit in-component loading UI (Skeleton) and
relies on React Suspense for data fetching. Please ensure there is a Suspense
fallback/pending UI at the route/layout level for this screen; otherwise users
may see a blank area (or an unhandled suspend) during initial load/refetch. If
the router doesn’t provide a fallback here, reintroduce a localized fallback
wrapper around the form content.
##########
src/routes/stream_routes/detail.$id.tsx:
##########
@@ -52,22 +52,21 @@ const StreamRouteDetailForm = (props: Props) => {
const { readOnly, setReadOnly, id } = props;
const { t } = useTranslation();
- const streamRouteQuery = useQuery(getStreamRouteQueryOptions(id));
- const { data: streamRouteData, isLoading, refetch } = streamRouteQuery;
+ const streamRouteQuery = useSuspenseQuery(getStreamRouteQueryOptions(id));
+ const { data: streamRouteData, refetch } = streamRouteQuery;
Review Comment:
Similar to the Route detail page, this removes the component’s `isLoading`
Skeleton path and depends on Suspense for the loading state. Confirm that a
Suspense fallback exists for this route so loading remains user-friendly and
doesn’t regress to a blank render.
##########
e2e/tests/upstreams.pass-host-reset.spec.ts:
##########
@@ -0,0 +1,312 @@
+/**
+ * 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 { upstreamsPom } from '@e2e/pom/upstreams';
+import { randomId } from '@e2e/utils/common';
+import { e2eReq } from '@e2e/utils/req';
+import { test } from '@e2e/utils/test';
+import { uiHasToastMsg } from '@e2e/utils/ui';
+import { expect } from '@playwright/test';
+
+import { deleteAllUpstreams } from '@/apis/upstreams';
+
+test.beforeAll(async () => {
+ await deleteAllUpstreams(e2eReq);
+});
+
+/**
+ * Test for GitHub issue #3294
+ * Bug: pass_host is reset to default value "pass" when editing upstream nodes
+ * @see https://github.com/apache/apisix-dashboard/issues/3294
+ */
+test('should preserve pass_host value when editing upstream nodes', async ({
+ page,
+}) => {
Review Comment:
The new spec uses indentation that appears inconsistent with existing e2e
tests in this repo (many use 2-space indentation). Running the repo’s formatter
(e.g., Prettier) on this file will improve readability and reduce noisy diffs
in future edits.
##########
src/routes/routes/detail.$id.tsx:
##########
@@ -93,10 +97,6 @@ const RouteDetailForm = (props: Props) => {
},
});
- if (isLoading) {
- return <Skeleton height={400} />;
- }
-
return (
Review Comment:
This change removes the explicit in-component loading UI (Skeleton) and
relies on React Suspense for data fetching. Please ensure there is a Suspense
fallback/pending UI at the route/layout level for this screen; otherwise users
may see a blank area (or an unhandled suspend) during initial load/refetch. If
the router doesn’t provide a fallback here, reintroduce a localized fallback
wrapper around the form content.
##########
e2e/tests/upstreams.pass-host-reset.spec.ts:
##########
@@ -0,0 +1,312 @@
+/**
+ * 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 { upstreamsPom } from '@e2e/pom/upstreams';
+import { randomId } from '@e2e/utils/common';
+import { e2eReq } from '@e2e/utils/req';
+import { test } from '@e2e/utils/test';
+import { uiHasToastMsg } from '@e2e/utils/ui';
+import { expect } from '@playwright/test';
+
+import { deleteAllUpstreams } from '@/apis/upstreams';
+
+test.beforeAll(async () => {
+ await deleteAllUpstreams(e2eReq);
+});
Review Comment:
The new spec uses indentation that appears inconsistent with existing e2e
tests in this repo (many use 2-space indentation). Running the repo’s formatter
(e.g., Prettier) on this file will improve readability and reduce noisy diffs
in future edits.
##########
src/routes/routes/detail.$id.tsx:
##########
@@ -15,9 +15,9 @@
* limitations under the License.
*/
import { zodResolver } from '@hookform/resolvers/zod';
-import { Button, Group, Skeleton } from '@mantine/core';
+import { Button, Group } from '@mantine/core';
import { notifications } from '@mantine/notifications';
-import { useMutation, useQuery } from '@tanstack/react-query';
+import { useMutation, useSuspenseQuery } from '@tanstack/react-query';
Review Comment:
This change removes the explicit in-component loading UI (Skeleton) and
relies on React Suspense for data fetching. Please ensure there is a Suspense
fallback/pending UI at the route/layout level for this screen; otherwise users
may see a blank area (or an unhandled suspend) during initial load/refetch. If
the router doesn’t provide a fallback here, reintroduce a localized fallback
wrapper around the form content.
##########
src/routes/stream_routes/detail.$id.tsx:
##########
@@ -82,10 +81,6 @@ const StreamRouteDetailForm = (props: Props) => {
},
});
- if (isLoading) {
- return <Skeleton height={400} />;
- }
-
return (
Review Comment:
Similar to the Route detail page, this removes the component’s `isLoading`
Skeleton path and depends on Suspense for the loading state. Confirm that a
Suspense fallback exists for this route so loading remains user-friendly and
doesn’t regress to a blank render.
--
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]
