lxbme opened a new pull request, #3458:
URL: https://github.com/apache/apisix-dashboard/pull/3458
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
- [ ] Improve performance
- [ ] Backport patches
**What changes will this PR take into?**
Opening a detail URL for an id that is not there was a dead end. The page
showed a loading skeleton, raised a toast reading etcd's `Key not found`, still
offered Edit and Delete for the resource that was not there, and gave no
statement that it was missing and no way back to the list. In a background tab
it never resolved at all.
### The retry change is global, and it is the precondition for everything
else
Admin API 404s are no longer retried (`src/config/global.ts`). This is not
scoped to detail pages — it changes retry behaviour for every query against the
Admin API, exactly as the existing 401 rule beside it already does, because
retrying a 404 cannot succeed either.
`src/components/page-slice/plugin_metadata/hooks.ts` already disabled retries
for 404 per-query with the same reasoning; this lifts that rule to the default.
It is also what makes the rest reachable. `query-core` pauses a retry
sequence while the tab is hidden — `canContinue()` in `retryer.js` requires
`focusManager.isFocused()`, which is `document.visibilityState !== 'hidden'` —
and a paused query never reaches `error`, so nothing is ever thrown. On a
background tab (⌘-click a row, or restore a session of tabs) the page therefore
sat on its skeleton indefinitely with Edit and Delete still clickable. With no
retry, the first 404 settles immediately, paused or not.
**Trade-off worth stating: this assumes a 404 is never transient.** That
holds here because these are single-resource GETs by id against etcd through
the Admin API — a 404 means the key does not exist at the moment of the read,
not a flaky upstream or a stale replica. There is no intermediate cache in this
path that could answer differently a moment later.
### Detail routes map that 404 to an explicit not-found panel
`genDetailErrorComponent` (`src/components/page/DetailNotFound.tsx`) is
wired as the `errorComponent` on all fourteen detail routes. It takes the id
from the URL params, names the missing resource, and links back to that
resource's list. It passes no `extra` to `PageHeader`, so Edit and Delete are
structurally absent rather than conditionally hidden. Non-404 errors fall
through to the app's generic error state; the two stream-route detail routes
keep their existing 400 "stream mode disabled" hint as that fallback.
### The not-found state must not outlive the resource
A 404'd query is re-thrown on remount rather than refetched — while the
query error-reset boundary is un-reset, `ensurePreventErrorBoundaryRetry` sets
`retryOnMount = false` and `shouldLoadOnMount` then refuses to fetch an errored
query, for as long as the entry lives (`gcTime`, five minutes past its last
observer). Landing on the panel therefore drops that cache entry outright, not
merely resets the boundary: `reset()` alone only flips a flag that the next
query mount clears again (`useBaseQuery` → `useClearResetErrorBoundary`), and
one trip through any list page does exactly that.
Without this, deleting a resource, opening its URL, and re-creating it under
the same id leaves the page still calling it deleted until a full reload. That
is the UI making a false statement about the user's data, so it is treated as a
correctness bug rather than a staleness nicety.
**Only 404s.** Every other failure keeps its cache entry on purpose:
recovering from a wrong admin key depends on `SettingsModal`'s debounced
`refetchQueries()` reaching exactly these observer-less error-state queries,
and `Query.isDisabled()` excludes any query that has not completed a fetch — so
a dropped (or `resetQueries`-reset) entry is one that a later refresh can never
reach again. A 404 is not something an admin key fixes.
### A 404 on a read no longer toasts
`src/config/req.ts` suppresses the toast for a 404 on a **GET** only; a 404
from a PUT/POST/DELETE still toasts, and write error handling is otherwise
untouched. A read 404 is the fact "this does not exist", which the page itself
now states — the gateway's raw wording as a red toast on top of that is noise.
Diagnosability is not lost elsewhere: list pages fetch through
`useSuspenseQuery` too, so a 404 there still throws to the app's error page and
remains visible on-page. Only the toast goes away.
The two hand-written 404 opt-outs at `src/apis/credentials.ts` and
`src/components/page-slice/plugin_metadata/hooks.ts` are now redundant with
this global rule. They are deliberately kept: they remain correct, they
document the intent at the call site, and removing them would widen the diff
for no behavioural gain.
### Why this touches `src/routes/__root.tsx`
A route-level error boundary intercepts *every* error thrown on that route,
not just the 404 it was added for. The app's own generic error state — message
plus a Retry that re-runs the failed load (#3418) — therefore had to move out
of `__root.tsx` into a shared `src/components/page/PageError.tsx` and become
the non-404 fallback. Without that, all fourteen detail pages would have
silently lost Retry, and the loss would have been easy to miss: TanStack's
default error text contains the app's own `error.title` as a substring, so a
text-only assertion still passes. Root behaviour is unchanged — `RootError`
still wraps it with the settings modal that is the only way into a fresh
install.
### Detail headings now name their resource
Every detail heading read `{{name}} Detail` — literally "Route Detail" — so
identifying which of a dozen similarly-named routes you had opened meant
reading the id field further down the form. Headings now read `Route · <id>`,
via a new `info.detail.titleWithId` key. The id comes from the URL, so the
heading needs no data: it renders during loading and in the not-found state
itself, with no layout shift. #3441 gave each page a distinct browser-tab
title; that key is shared with the tab title and two tab labels and is left
untouched.
Twelve detail components changed, covering fourteen views (the two nested
service routes reuse the exported `RouteDetail` / `StreamRouteDetail`). Twelve
`e2e/pom/*.ts` helpers were updated to match: they asserted the old exact
heading text, and Playwright's `name` option matches substrings, so `'Route
Detail'` also matched "Stream Route Detail" — the replacement `/^Route · /u` is
start-anchored and case-sensitive, so it is slightly stricter than what it
replaces.
One consequence worth flagging rather than leaving to be discovered: because
the panel reuses the live heading format, those `isDetailPage` helpers now also
match on a not-found page. That is intentional — the missing resource's
identity belongs in the heading — and the helpers pair the heading with a URL
assertion; they were never proof that data loaded.
New strings nest inside the existing `error` and `info.detail` objects,
translated in all five locales. The German and Spanish wordings deliberately
carry no article: `{{name}}` interpolates thirteen resource nouns of differing
grammatical gender, so any fixed article would be wrong for most of them.
**Related issues**
Part of #3453
**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?
- [ ] 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:
- `src/utils/error.test.ts` covers `isNotFoundError`: a 404 axios error,
non-404 statuses (401/400/500/503), an axios error with no `response` at all (a
network failure is not an absence), and non-axios values.
- `e2e/tests/regression/detail.not-found.spec.ts` — four tests. A missing
route is **requested exactly once** (the load-bearing assertion: a panel can
render correctly while the retry storm survives untouched), states its absence,
raises no toast, carries the id in its heading, exposes no Edit or Delete
control, keeps the nav, and its back link reaches the list. The not-found state
does not outlive the resource: the route is re-created under the same id and
reached again *within the same SPA session*, and the form must render — a
`page.goto` there would build a fresh cache and pass on the unfixed code. A
missing consumer covers the `$username` param shape, and a missing route under
a service covers the nested back link.
- `e2e/tests/regression/detail.heading-identity.spec.ts` asserts the live
heading is exactly `Route · <id>` on the route detail page, with a resource
name deliberately distinct from the id so it cannot pass by matching the name,
and that edit mode's heading is unchanged.
- `e2e/tests/regression/secrets.detail-fetch-error.spec.ts` was extended to
**click** Retry after a network-level failure and assert the form comes back,
not merely to assert the button is visible — the boundary that catches such
errors moved from the root to the route, so `router.invalidate()` plus the
query-error reset now run at a level they previously did not.
Verified: `pnpm test` (104/104), `pnpm lint`, `pnpm exec tsc -b` and a
production `pnpm build` all clean. The full Playwright suite was run on a local
merge of this branch together with the three other in-flight PRs (#3454, #3456,
#3457), which merged without conflict: 225 passed, 4 failed, every failure
environmental and reproducible without this change — two Monaco editor render
races, one Chrome launch timeout where no application code ran, and one spec
that restarts a compose project absent from this machine; both plugin_metadata
specs pass on isolated rerun. The stream-route 400 fallback, which cannot be
exercised locally by that spec, was checked by hand in both directions: a
disabled stream mode still shows the gateway's hint, a missing id shows the
panel.
--
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]
