Copilot commented on code in PR #3295:
URL: https://github.com/apache/apisix-dashboard/pull/3295#discussion_r2911141617
##########
src/routes/routes/detail.$id.tsx:
##########
@@ -62,12 +62,20 @@ const RouteDetailForm = (props: Props) => {
const routeQuery = useQuery(getRouteQueryOptions(id));
const { data: routeData, isLoading, refetch } = routeQuery;
+ // Compute initial form values from route data
+ const formDefaults = routeData?.value
+ ? produceVarsToForm(
+ produceToUpstreamForm(routeData.value.upstream || {}, routeData.value)
+ )
+ : undefined;
+
const form = useForm({
resolver: zodResolver(RoutePutSchema),
shouldUnregister: true,
shouldFocusError: true,
mode: 'all',
disabled: readOnly,
+ defaultValues: formDefaults,
});
Review Comment:
`defaultValues` for `useForm` are only applied on the first render. Since
this form is driven by `useQuery` (not suspense) and `routeData` is initially
`undefined` on a cold load, `formDefaults` will typically be `undefined` during
the form initialization and won’t actually seed the form defaults. If the
intent is to ensure fetched values act as defaults for re-registered fields
with `shouldUnregister: true`, consider switching to `useSuspenseQuery` here,
or explicitly updating defaults via `form.reset(formDefaults)` when data
arrives (or remounting the form via a `key` once defaults are ready).
##########
src/routes/upstreams/detail.$id.tsx:
##########
@@ -68,11 +71,14 @@ const UpstreamDetailForm = (
refetch,
} = useSuspenseQuery(getUpstreamQueryOptions(id));
- const form = useForm({
- resolver: zodResolver(FormPartUpstreamSchema),
+ const formDefaults = produceToUpstreamForm(upstreamData) as
FormPartUpstreamType;
+ const form = useForm<FormPartUpstreamType>({
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ resolver: zodResolver(FormPartUpstreamSchema) as any,
shouldUnregister: true,
Review Comment:
`useForm`'s `resolver` is being cast to `any` (and suppressed via eslint),
which removes type-safety for validation and can hide real schema/form
mismatches. Prefer fixing the resolver typing instead of casting (e.g. align
the `useForm` generic with the schema via `z.infer`/`z.input`, or parameterize
`zodResolver` if needed) so the form stays fully typed end-to-end.
##########
src/routes/stream_routes/detail.$id.tsx:
##########
@@ -61,6 +61,7 @@ const StreamRouteDetailForm = (props: Props) => {
shouldFocusError: true,
mode: 'all',
disabled: readOnly,
+ defaultValues: streamRouteData?.value,
});
Review Comment:
Similar to routes detail: because this page uses `useQuery`,
`streamRouteData` is `undefined` on the initial render on a cold load, and
React Hook Form won’t pick up later `defaultValues` changes. If this is meant
to prevent values from reverting when fields are unregistered/re-registered
(`shouldUnregister: true`), you likely need to ensure defaults are set after
the query resolves (e.g. via suspense, a keyed remount, or resetting defaults
when data arrives).
##########
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:
This spec clears *all* upstreams in `beforeAll`. With Playwright configured
as `fullyParallel: true` (and multiple workers locally), this can race with
other upstream-related specs running in parallel and make the suite flaky by
deleting resources created by other tests. Prefer cleaning up only the
upstreams created by this spec (e.g. track created IDs and delete them in
`afterEach`/`afterAll`), and/or wrap the file in `test.describe.configure({
mode: 'serial' })` if it must touch shared global state.
--
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]
