test(e2e): regression suite + form-state refactor (fixes #3293, #3342, #3344, #3370)#3391
Open
moonming wants to merge 3 commits into
Open
test(e2e): regression suite + form-state refactor (fixes #3293, #3342, #3344, #3370)#3391moonming wants to merge 3 commits into
moonming wants to merge 3 commits into
Conversation
## New E2E test specs (51 tests across 4 directories) ### regression/ (19 specs) - consumers.configure-next-no-crash — opening API-created consumer detail doesn't crash - consumers.hyphen-username — consumer username with hyphens accepted (APISIX 3.13+) - consumers.labels-displayed — consumer labels preserved after save - credentials.list-no-crash-on-network-error — no crash when request is aborted (#3370) - form.add-save-button-on-overflow — Add button reachable after scrolling - form.cancel-unsaved-warning — Edit→Cancel warns on dirty form (#3344) - form.mutation-failure-error — Admin API 5xx surfaces a visible error toast - form.plugin-delete-confirmation — plugin delete requires confirmation (#3342) - form.plugin-drawer-close-warns — closing Add Plugin drawer with edits warns - plugins.editor-schema-fetch-failure — plugin editor recovers from schema fetch error - routes.empty-plugin-config — plugins with {} config survive a no-op save (#3269) - routes.upstream-id-only — route with upstream_id reference (no inline nodes) - routes.vars-editor — vars field is optional; API-seeded vars render on detail - ssls.labels-after-create — SSL labels visible after creation - upstreams.discovery-args — discovery_type/service_name/discovery_args persist (#3376) - upstreams.node-sync-on-save — [fixme] typed node values reach API without blur (#3293) - upstreams.percent-undefined-no-crash — zero-weight upstream renders without crash (#3381) - validation.multi-auth-schema — multi-auth plugin with valid config accepted (#3289) - validation.zOneOf-single-field — service with inline-upstream-only saves cleanly ### edge/ (10 specs) - plugins.invalid-json-monaco — invalid JSON cannot be saved - plugins.large-config-roundtrip — 10 KB plugin config survives a UI round trip - routes.long-name — 100-char name accepted; 1000-char rejected - routes.nonexistent-service-id — non-existent service_id rejected with error - routes.same-uri-different-method — two routes share URI with different methods - routes.special-uri-chars — wildcard and path-parameter URIs accepted - routes.unicode-emoji-name — CJK + emoji + RTL name persists exactly - routes.whitespace-only-name — whitespace-only name rejected - routes.xss-name-payload — XSS payload rendered as text, not executed - upstreams.port-boundaries — port 65535 accepted; 0 and 65536 rejected ### integration/ (3 specs) - empty-state.all-resources — empty-state pages show no raw i18n keys - i18n.lang-switch-no-crash — switching every offered language doesn't crash - lifecycle.delete-referenced-upstream — deleting in-use upstream surfaces API error ### bulk/ (2 specs) - routes.bulk-100.list-render — pagination correct with 100 rows - routes.bulk-1000.list-search — first page renders within 5 s with 1 000 rows ## New test utilities - e2e/utils/clean.ts — safeClean() swallows stream-mode-disabled 400s - e2e/utils/bulk.ts — bulk seed / delete helpers for Admin API - e2e/utils/ui/crash.ts — watchForCrashes() captures pageerror events ## Product bug fixes ### Fix #3342 — plugin delete requires confirmation - src/components/form-slice/FormItemPlugins/PluginCard.tsx Added Mantine confirmModal before firing onDelete. - Updated tests: routes.crud-all-fields, plugin_metadata.crud-required/all-fields, consumer_groups.crud-required-fields, ssls.crud-all/required-fields ### Fix #3344 — Cancel on dirty detail form warns before discarding - src/hooks/useEditCancelGuard.tsx (new hook) - Applied to all 11 resource detail pages: routes, upstreams, services, consumers, consumer_groups, credentials, global_rules, plugin_configs, protos, secrets, ssls, stream_routes ### Fix #3370 — credentials list crash on undefined response - src/apis/credentials.ts — guard for e.response === undefined ### Fix whitespace-only name validation - src/types/schema/apisix/common.ts — .trim().min(1) on name fields ## Infrastructure / test-helper fixes - src/apis/stream_routes.ts — deleteAllStreamRoutes swallows "stream mode disabled" 400s so deleteAllServices works in non-stream deployments - src/routes/__root.tsx — hide devtools in test mode (avoids UI interference) - e2e/utils/ui/index.ts — uiFillMonacoEditor uses setValue() instead of fill() to avoid auto-bracket corruption by Monaco - e2e/utils/ui/upstreams.ts — uiFillUpstreamRequiredFields: derive page from ctx always (was conditional on optional 3rd arg), add force:true + 300 ms settle between rapid Add Node clicks; remove the now-unused _page parameter - e2e/server/apisix_conf.yml — strip redundant comments (cosmetic)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has checked 331 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 283 | 1 | 47 | 0 |
Click to see the invalid file list
- e2e/server/apisix_conf.yml
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
| # limitations under the License. | ||
| # | ||
|
|
||
| apisix: |
There was a problem hiding this comment.
Suggested change
| apisix: | |
| # 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. | |
| apisix: |
Addresses HIGH/MEDIUM/LOW findings from the independent audit on PR #3391. - **5.1 HIGH** — Plugin delete confirmation test now covers the dismiss path: clicking the dialog's Cancel button must leave the plugin intact. A future regression that wires Cancel to the delete handler would slip past the original "modal appeared" check. - **1.1 HIGH** — Documented the deliberate "always show modal" tradeoff in useEditCancelGuard. Tried implementing the audit's "skip on clean form" suggestion but it's not reliable with the current form lifecycle: toggling `disabled` on Edit re-renders controlled inputs and fires spurious dirty events, and the useEffect-driven `form.reset(...)` wipes the dirty flag mid-session. The proper fix requires restructuring the form lifecycle (tracked as a follow-up). For now, one extra click is a far better failure mode than silently losing an edit. The corresponding test (test 2) now pins the dismiss path instead of asserting the not-yet-implemented skip-on-clean behavior. - **1.2 MEDIUM** — Tightened `deleteAllStreamRoutes` guard to require both a 4xx status (or no response) AND the "stream mode" text. The prior version would have swallowed any error whose message happened to contain "stream mode" regardless of status code. - **3.2 LOW** — Restored the ASF license header to apisix_conf.yml.
The three components below all shared the same architectural anti-pattern that caused #3293: a MobX `useLocalObservable.__map` cache mirroring the form/query value, plus a `useEffect([..., value])` pushing value DOWN into the cache, plus a `save()` pushing cache UP via fOnChange. The bidirectional sync raced on every fast update — typing into a cell and clicking Save without blurring first could lose data; an in-flight form update could reinitialize the mirror from a stale snapshot and overwrite a just-saved plugin config. This change collapses all three to a single source of truth. **FormItemPlugins/index.tsx** - Removed `useLocalObservable.__map` and the `useEffect([..., rawObject])` that re-derived it - `selected` now reads `Object.keys(rawObject)` directly from the RHF controller value - `handleUpdate` / `handleDelete` construct the new plugins object inline and push via `fOnChange` — no intermediate cache to go stale - Only transient UI state (curPluginName, mode, drawer open flags, search) stays in component state via useState **PluginMetadata.tsx** - Removed `__map` / `__schemaMap` and the `useDeepCompareEffect` that reinitialized them — the page now reads `pluginInfoMap` and `hasConfigNames` directly from the `usePluginMetadataList()` query - `curPlugin` and `curPluginSchema` are derived per-render from `pluginInfoMap.get(curPluginName)` - mutations + refetch flow is unchanged **FormPartUpstream/FormItemNodes.tsx (fixes #3293)** - Removed `useLocalObservable.ob` and the `useEffect([ob, value])` that re-derived row ids and disrupted the EditableProTable's editable lifecycle (the failure mode of the prior `useDebouncedCallback` attempt) - React `useState` holds the table's display rows, initialized once from the form value; subsequent external value changes are handled by the parent re-keying this component, which is the normal RHF detail-page reset flow - Every onValuesChange / add / remove pushes UP to RHF synchronously via `fOnChange`. No down-sync, so no feedback loop - Last-resort DOM flush: a document-level `mousedown` capture-phase listener walks the cell inputs and pushes their CURRENT visible values to RHF whenever a mousedown lands OUTSIDE the table. This is what makes the #3293 regression test pass deterministically — ant-design's `valueType: 'digit'` cells don't commit typed values to Antd Form's internal state until blur, but `mousedown` fires BEFORE the click handler on the Submit button, so RHF sees the correct values Unblocks `e2e/tests/regression/upstreams.node-sync-on-save.spec.ts`, which was previously `.fixme`'d. All affected CRUD specs (routes/services/upstreams/plugin_metadata/ consumers/plugin_configs) plus the full regression / edge / integration / bulk suites pass.
moonming
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two motivations bundled into one PR because they're tightly coupled:
What's in this PR
1. New Playwright E2E tests (52 tests)
e2e/tests/regression/e2e/tests/edge/e2e/tests/integration/e2e/tests/bulk/All 52 pass locally with
--workers=1(CI configuration).2. Bug fixes
#3293 — Upstream nodes lost on submit-without-blur
Symptom: user types a host/port/weight, clicks Save without clicking anywhere else first, the typed values are discarded and the previous (or empty) values are submitted to the Admin API.
Root cause:
FormItemNodes.tsxmirrored the form value into a MobXuseLocalObservable.__map, with auseEffect([ob, value])syncing down andob.save() → fOnChangesyncing up. The bidirectional loop raced any update faster than ~100 ms; meanwhileant-design'svalueType: 'digit'cells only commit typed values to Antd Form on blur. Submit-without-blur missed both gates.Fix (see commit
e93f780c):useStateonly holds display rows, initialized once from the form value.onValuesChange/ add / remove pushes UP synchronously viafOnChange. NouseEffectpushes value DOWN — no feedback loop.mousedowncapture-phase listener flushes DOM input values to RHF whenever mousedown lands outside the table. Because mousedown fires before the Submit button's click handler, RHF sees the user's typed-but-not-blurred values regardless of Antd Form's commit timing.Unblocks
e2e/tests/regression/upstreams.node-sync-on-save.spec.ts(was.fixme, now stable 5/5 repeats in headless).#3293 latent variants — same anti-pattern, two more components
The independent audit on this PR flagged that
FormItemPlugins/index.tsxandpage-slice/plugin_metadata/PluginMetadata.tsxused the sameuseLocalObservable.__map+useEffect([..., value])+save → fOnChangeshape.FormItemPluginshad the same submit-time data-loss window (a parallel re-render could re-initialize__mapfrom a stalerawObjectand overwrite a just-saved plugin config).PluginMetadatahad a smaller mutation-vs-refetch window but the structural pattern was identical.Both refactored to the same single-source-of-truth shape in
e93f780c:FormItemPlugins:selected = Object.keys(rawObject)directly.handleUpdate/handleDeleteconstruct the new plugins object inline and push viafOnChange. UI state (curPluginName, drawer flags, search) moves touseState.PluginMetadata: readspluginInfoMapandhasConfigNamesdirectly from theusePluginMetadataList()query.curPluginandcurPluginSchemaare derived per-render.#3342 — Plugin delete confirmation
PluginCard.tsx: wraps the destructiveonDeletein a MantineopenConfirmModal. Regression test asserts BOTH the dismiss path (Cancel keeps the plugin) AND the confirm path (Delete removes it).#3344 — Cancel on Edit form warns before discarding
New
useEditCancelGuardhook applied to all 11 resource detail pages (routes, upstreams, services, consumers, consumer_groups, credentials, global_rules, plugin_configs, protos, secrets, ssls, stream_routes).The hook always shows the modal — even on a visually-clean form. This is a deliberate tradeoff documented in the hook source: the pattern
useForm({ disabled: readOnly })+useEffect(() => form.reset(...))makesformState.isDirtyunreliable (togglingdisabledfires spurious dirty events; the useEffect-driven reset wipes the flag mid-session). One extra click is a far better failure mode than silently losing an edit. Adding a reliable "skip-on-clean" path requires restructuring the form lifecycle and is filed as follow-up cleanup.#3370 — Credentials list crash on undefined network response
src/apis/credentials.ts: guards againste.response === undefined(aborted/timed-out request) so the consumer detail page doesn't crash into the global error boundary. Regression test usespage.routeto abort the credentials request and asserts no crash.Whitespace-only name validation
src/types/schema/apisix/common.ts: a Zod.refine()rejects names consisting only of whitespace. Pre-existing names (including non-whitespace-only with leading/trailing spaces) continue to validate — only\" \"and similar are blocked.3. Test infrastructure improvements
These are reusable utilities that earlier PRs (#3377 etc.) would have benefited from:
deleteAllStreamRoutesdoesn't bring down the cleanup chain when APISIX is deployed withoutproxy_mode: http&stream. The 400 "stream mode is disabled" response is now swallowed (gated by both status code 4xx AND message text containing "stream mode"). LetsdeleteAllServices(which transitively cleans stream routes) work in non-stream environments.uiFillMonacoEditorusessetValue()instead offill()to avoid Monaco's auto-bracket pairing corrupting JSON inputs.uiFillUpstreamRequiredFieldsderivesPagefrom theLocatorcontext and adds explicit settle waits between rapid Add Node clicks, eliminating a race that broke multiple specs.__root.tsxhides devtools in test mode to keep them out of the way of Playwright queries.safeClean()(e2e/utils/clean.ts),bulkCreate*/bulkDelete*(e2e/utils/bulk.ts),watchForCrashes()(e2e/utils/ui/crash.ts).Local verification
--workers=1(CI mode)pnpm lintclean with--max-warnings=0pnpm tsc --noEmitclean#3293regression test repeats green 5/5 in headless modeWhat to watch in CI
workers: 1) in CI perplaywright.config.ts, so the local serial results should hold.e2e/tests/stream_routes.*requireapisix.proxy_mode: http&stream. The dashboard's owne2e/server/docker-compose.ymlconfigures this; non-stream APISIX deployments will skip these via the new error-swallowing indeleteAllStreamRoutes.secrets.crud-all-fieldsexpects a\"Secret Manager\"label that the UI doesn't currently render — pre-existing test gap, unrelated to this PR. Recommend tracking as a separate issue.Audit history
After the initial push, an independent audit agent reviewed the diff cold (no shared context). Findings addressed:
9c4355aauseEditCancelGuardalways shows modal — UX concern9c4355aadeleteAllStreamRoutesregex-only guard could swallow unrelated errors9c4355aaapisix_conf.ymllost its ASF license header9c4355aaFormItemPluginsandPluginMetadatause the same__mapanti-pattern asFormItemNodes#3293.fixmetest unblockede93f780cCommit map