Optional nested form sections and improved JSON merge semantics - #26
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d501345d74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| control={control} | ||
| name={np('scheme')} | ||
| label="Scheme" |
There was a problem hiding this comment.
With the defaultValue removed here, FormItemSelect falls back to genControllerProps(props, []), so a new upstream or inline upstream with no form default registers scheme (and the same pattern repeats for type, hash_on, and pass_host) as an empty array. APISIX.Upstream only accepts those fields as string literals when present, so submitting an otherwise untouched upstream form fails validation instead of letting APISIX defaults apply. Either keep a valid string default or make the single-select fallback undefined.
Useful? React with 👍 / 👎.
| const merged: Record<string, unknown> = { ...originalValue }; | ||
|
|
||
| for (const [key, value] of Object.entries(formValue)) { | ||
| if (value === undefined) continue; |
There was a problem hiding this comment.
When the new remove controls clear a nested optional block, they mark it dirty by setting the value to undefined, but this guard drops that dirty value before merging with rawData. In edit mode FormJsonTabs submits mergeEditablePayloadByDirty for visual saves, so removing route timeout, stream-route protocol, or upstream timeout/keepalive_pool/tls keeps the original block in the saved payload and the user cannot actually delete it from the form. Dirty undefined values need to be represented as deletions instead of skipped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR improves APISIX resource editing UX by introducing toggleable optional nested configuration sections in several forms, and by changing how visual-form submissions are merged with existing (raw) resource data to only apply user-edited (dirty) fields.
Changes:
- Added optional enable/remove UI for nested config blocks (e.g., upstream
timeout/keepalive_pool/tls, routetimeout, stream routeprotocol). - Introduced
mergeEditablePayloadByDirtyand wired it intoFormJsonTabsso visual-editor saves merge only dirty fields when editing existing resources. - Adjusted JSON/switch form controls and upstream select defaults handling.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/apisixEditable.ts |
Adds dirty-field-aware merge helper used to avoid overwriting untouched nested fields. |
src/components/form/Switch.tsx |
Switch value coercion and controller defaults adjustment. |
src/components/form/JsonInput.tsx |
Changes controller default fallback behavior for JSON inputs when toObject is enabled. |
src/components/form/FormJsonTabs.tsx |
Uses dirty-field merge for form-source saves and adds a submit “source” distinction. |
src/components/form-slice/FormPartUpstream/index.tsx |
Adds optional nested sections for upstream config and removes several select defaultValues. |
src/components/form-slice/FormPartStreamRoute/index.tsx |
Adds optional protocol section enable/remove behavior. |
src/components/form-slice/FormPartRoute/index.tsx |
Adds optional timeout section enable/remove behavior for routes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const dirtyRecord = isRecord(dirtyFields) ? dirtyFields : {}; | ||
| const merged: Record<string, unknown> = { ...originalValue }; | ||
|
|
||
| for (const [key, value] of Object.entries(formValue)) { | ||
| if (value === undefined) continue; | ||
|
|
||
| const keyDirty = dirtyRecord[key]; | ||
| if (keyDirty === true) { | ||
| merged[key] = value; | ||
| continue; | ||
| } | ||
|
|
||
| if (isRecord(keyDirty) && isRecord(value)) { | ||
| const previous = merged[key]; | ||
| const base = isRecord(previous) ? previous : {}; | ||
| const nested = mergeEditablePayloadByDirty(base, value, keyDirty); | ||
| if (Object.keys(nested as Record<string, unknown>).length > 0) { | ||
| merged[key] = nested; | ||
| } | ||
| } | ||
| } |
| const { objValue = {} } = props; | ||
| const { | ||
| controllerProps: rawControllerProps, | ||
| restProps: { toObject, label, description, ...restProps }, | ||
| } = genControllerProps(props, props.toObject ? objValue : ''); | ||
| } = genControllerProps(props, props.toObject ? undefined : ''); | ||
| const controllerProps = useMemo(() => { |
| <FormItemSelect | ||
| control={control} | ||
| name={np('scheme')} | ||
| label="Scheme" |
| <FormItemSelect | ||
| control={control} | ||
| name={np('type')} | ||
| label="Type" | ||
| defaultValue={APISIX.UpstreamBalancer.options[0].value} | ||
| data={APISIX.UpstreamBalancer.options.map((v) => v.value)} |
| <FormItemSelect | ||
| control={control} | ||
| name={np('hash_on')} | ||
| label="Hash On" | ||
| defaultValue={APISIX.UpstreamHashOn.options[0].value} | ||
| data={APISIX.UpstreamHashOn.options.map((v) => v.value)} |
| <FormItemSelect | ||
| control={control} | ||
| name={np('pass_host')} | ||
| label="Pass Host" | ||
| defaultValue={APISIX.UpstreamPassHost.options[0].value} | ||
| data={APISIX.UpstreamPassHost.options.map((v) => v.value)} |
Motivation
Description
timeout,keepalive_pool, andtlsin upstreams and to toggleprotocolfor stream routes andtimeoutfor routes, withButtoncontrols to enable or remove each section and appropriateuseWatch,unregister, andsetValuehandling.hasFieldValue,hasProtocolValue, andhasTimeoutValueto detect whether nested objects/fields should be considered enabled.mergeEditablePayloadByDirtyinutils/apisixEditable.tsand wired it intoFormJsonTabsso merges from form editor only include fields that are actually dirty, while JSON-source submissions still perform the full merge viamergeEditablePayload.FormItemJsonInputnow passes controller defaults differently to supporttoObject,FormItemSwitchcoerces checked state with!!value, and several upstream select inputs had theirdefaultValueremoved to rely on form defaults.FormJsonTabsto pass asourceflag tosafeSubmit, to set the pending payload correctly, and to reset/submit JSON editor flows accordingly.Testing
tsc --noEmitand it completed successfully.yarn testand existing unit tests passed.yarn build) to ensure components compile and no runtime type errors occurred.Codex Task