Conversation
…3294) Also added E2E regression test e2e/tests/upstreams.pass-host-reset.spec.ts
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the pass_host field in Upstream configurations was incorrectly resetting to its default value ("pass") when editing Upstream Nodes. The issue occurred because forms using shouldUnregister: true would re-register fields without proper default values during re-renders.
Changes:
- Added explicit
defaultValuestouseFormhooks in detail pages to preserve field values during form re-registration - Implemented comprehensive E2E tests to verify
pass_hostvalue preservation across different scenarios - Applied the fix consistently across Upstream, Route, Service, and Stream Route detail pages
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/upstreams/detail.$id.tsx | Added formDefaults computed from fetched upstream data and passed to useForm as defaultValues |
| src/routes/routes/detail.$id.tsx | Computed formDefaults from route data including upstream configuration and passed to useForm |
| src/routes/services/detail.$id/index.tsx | Added defaultValues from service data to useForm hook |
| src/routes/stream_routes/detail.$id.tsx | Added defaultValues from stream route data to useForm hook |
| e2e/tests/upstreams.pass-host-reset.spec.ts | Added comprehensive E2E tests verifying pass_host preservation for both "node" and "rewrite" values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…fety in upstream form
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Hi @Baoyuantop I have addressed the comments by copilot. |
There was a problem hiding this comment.
Good root cause analysis — the shouldUnregister: true + missing defaultValues interaction is a classic react-hook-form pitfall and the fix direction is correct.
However there are a few issues that need to be addressed before this can be merged. See inline comments below.
Summary:
| Category | Items |
|---|---|
| Must fix | Missing Suspense boundaries (services, upstreams), e2e deleteAllUpstreams still present |
| Suggestion | Suspense fallback style inconsistency, consider useMemo for formDefaults |
Sure, I will go through the comments and apply the fix. |
|
Hi @DSingh0304, please fix failed CI |
Yes, I will fix the CI in a while. |
…data initialization
|
The CI was failing primarily due to a synchronization race condition in the dynamic Upstream Nodes form and missing UI state initialization during resource updates. In the FormItemNodes component (Upstream Nodes table), field values were being synced to the main form only on "blur" or "click outside" events. High-speed E2E tests often clicked the "Submit" button before these events could trigger, resulting in empty or stale data being sent. When editing existing Routes/Services, internal UI flags (like __checksEnabled) used to manage form sections were not initialized from the backend data, causing validation to fail or sections to be hidden incorrectly. Additionally, sending empty nested objects (e.g., timeout: {}) caused schema rejection on the APISIX backend. Fix Details: Real time Sync: Refactored FormItemNodes.tsx to sync with the form state immediately on every keystroke (onChange) and row operation (Add/Delete), ensuring the form state is always up-to-date before submission. UI State Hydration: Introduced produceToNestedUpstreamForm to correctly map backend API responses to the dashboard's internal UI-specific form structure, ensuring toggles and section states are restored on edit. Recursive Sanitization: Enhanced the data pipeline with produceRmEmptyUpstreamFields to recursively prune empty nested objects before they reach the backend, preventing schema validation errors. Stability: Adjusted Monaco Editor mounting in Editor.tsx to allow test environment access during development mode, improving E2E stability. |
|
Hi @Baoyuantop I tried fixing the CI can you run the checks again |
Why submit this pull request?
What changes will this PR take into?
This PR fixes a bug where the
pass_hostfield in Upstream configurations was incorrectly resetting to its default value ("pass") when editing Upstream Nodes.Problem:
The forms in the detail pages (Upstream, Route, Service, Stream Route) use
shouldUnregister: true. When editing nodes, parts of the form re render and re register fields. Without explicitdefaultValuesprovided touseForm, these fields were falling back to their component leveldefaultValueprops (which is"pass"forpass_host) instead of retaining their actual state.Solution:
useForminitialization insrc/routes/upstreams/detail.$id.tsx,src/routes/routes/detail.$id.tsx,src/routes/services/detail.$id/index.tsx, andsrc/routes/stream_routes/detail.$id.tsx.defaultValues(sourced from the fetched data) with theuseFormhook. This ensures that fields retain their correct values even when they are unregistered and re registered.Related issues
fix #3294
Checklist: