lxbme opened a new pull request, #3443:
URL: https://github.com/apache/apisix-dashboard/pull/3443
Please answer these questions before submitting a pull request, **or your PR
will get closed**.
**Why submit this pull request?**
- [x] Bugfix
- [x] New feature provided
- [x] Improve performance
**What changes will this PR take into?**
This addresses the "Dirty-form navigation is fully unguarded" item of the
frontend review in #3417. Three related defects travelled together in that one
checkbox, and this PR fixes all three:
1. **Nothing guarded navigation away from a form with unsaved edits.** There
was not a single `useBlocker` / `beforeunload` in the app: a sidebar click,
browser Back, tab close, or reload silently discarded in-flight edits.
2. **The 14 "add" pages had no Cancel button at all** — navigating away (the
unguarded path above) was the *only* way to abandon a half-filled form.
3. **The Edit → Cancel confirmation interrogated the user unconditionally**,
even on a form they had not changed. Its own comment claimed react-hook-form's
`isDirty` could not be trusted under the `disabled`-toggling edit architecture.
### What the PR adds
- **`src/utils/form-dirty.ts`** — a pure `isFormDirty(defaults, values)`
that normalises both sides before comparing (drops UI-only `__`-prefixed flags,
`create_time`/`update_time`, and empty `""` / `[]` / `{}` values; keeps plugin
configs verbatim so an added-but-empty plugin still counts as an edit). This is
needed because the raw `isDirty` flag reports several add pages as dirty before
the user types anything — their widgets normalise `undefined` to empty values
on mount.
- **`src/hooks/useUnsavedChangesGuard.tsx`** — wires that check to TanStack
Router's `useBlocker` (a confirm modal on in-app navigation) and
`enableBeforeUnload` (the browser's native prompt on reload / tab close). A
`bypass()` handle lets a page's own post-submit redirect through.
- **`FormCancelBtn`** (in `src/components/form/Btn.tsx`) — a Cancel button
rendered as a router link, so a dirty form routes Cancel through the same guard
with no separate code path. Added to all 14 add routes; each returns to its
list page (nested service sub-routes return to the correct sub-list).
- **12 detail pages** mount the guard with `disabled: readOnly`, and
**`useEditCancelGuard`** now consults `isFormDirty` — a clean form cancels
without a modal, a dirty one still confirms. The two hooks share one modal
definition and one definition of "changed".
### Verifying the claims before fixing
The old `useEditCancelGuard` comment asserted `isDirty` was untrustworthy
(spurious change events from the `disabled` toggle; the reset-on-refetch effect
wiping dirtiness). Measured against all 12 detail pages, neither reproduces:
entering edit mode leaves the form clean, and an ordinary refetch returns the
same `data` reference (react-query structural sharing) so the reset effect
never re-runs. That is why the Cancel modal can safely become conditional
without the larger "mount the form only in edit mode" restructure the review
suggested (left out of scope here).
### Two latent bugs surfaced while building this
- **Object-form upstream nodes read spurious-dirty.** A detail page whose
upstream stores nodes as the Admin API's object map (`{ "host:port": weight }`)
compared unequal to the array form the nodes widget produces on mount, so any
edit page with an upstream looked dirty with zero input. `form-dirty.ts`
canonicalises nodes on both sides before comparing (unit-tested for
object-map/array equality, IPv6, port-less, and genuine node edits).
- **`nanoid()`-seeded ids were unstable across renders.** Five add pages
seeded `defaultValues: { id: nanoid() }` with `nanoid()` called in the render
body, so `formState.defaultValues.id` and `getValues().id` held two different
ids and the pristine form read dirty. Fixed by generating the id once via
`useState(() => nanoid())`. This only stabilises an already-present default —
the submitted payload is unchanged.
### Performance
The blocker's `shouldBlockFn` is memoised so `useBlocker` subscribes once
rather than re-subscribing (and re-running the dirty check) on every render.
**Related issues**
Part of #3417
**Checklist:**
- [x] Did you explain what problem does this PR solve? Or what new features
have been added?
- [x] Have you added corresponding test cases?
- [x] Have you modified the corresponding document?
- [x] Is this PR backward compatible? If it is not backward compatible,
please discuss on the mailing list first
Tests: unit coverage for the dirty check (empty-value normalisation,
plugin-config edits, node canonicalization) in `src/utils/form-dirty.test.ts`;
e2e coverage in `e2e/tests/regression/form.unsaved-changes-guard.spec.ts`
(dirty add/detail navigation warns; pristine add pages — including a
`nanoid`-seeded one — navigate without a prompt; Cancel button behaviour;
successful submit is not blocked). The `beforeunload` path has no e2e assertion
because headless Chrome auto-dismisses the native dialog; it was verified
manually (pristine page does not arm it, an edited page does). No documentation
changes are needed — this is behavioural.
One question for reviewers: the reused `info.unsaved.content` copy reads
"You have unsaved changes. Are you sure you want to close?", which is slightly
stilted for a navigation rather than a modal close. Rewording it touches the
de/es/tr translations, so I left it as-is rather than changing wording I cannot
verify — happy to update if you'd prefer.
--
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]
