lxbme opened a new pull request, #3459:
URL: https://github.com/apache/apisix-dashboard/pull/3459
Please answer these questions before submitting a pull request, **or your PR
will get closed**.
**Why submit this pull request?**
- [ ] Bugfix
- [x] New feature provided
- [ ] Improve performance
- [ ] Backport patches
**What changes will this PR take into?**
`upstream_id`, `service_id`, `plugin_config_id` and `group_id` rendered as
plain text inputs holding a bare id. There was no name resolution and nothing
to click, so answering "which upstream is this route actually using?" meant
copying the id, navigating to Upstreams, and searching by hand.
Each field now resolves its id and offers a link to the referenced resource.
Four fields in two files cover the four referenceable resource types —
`FormPartRoute/index.tsx` (`upstream_id`, `plugin_config_id`, `service_id`) and
`FormPartConsumer.tsx` (`group_id`) — because the form sections that hold these
fields are shared between routes, stream routes, services and consumers. The
change is far smaller than "every resource with an id field": it targets
exactly the four fields that reference another top-level resource.
### Why the control is an anchor, not `RouteLinkBtn`
The repo's existing `RouteLinkBtn` is a Mantine `Button`. `FormSection`
renders a `<fieldset>` and passes `disabled` when the section is read-only —
most visibly in the nested service → route view, where the whole form is
disabled while browsing. A disabled `<fieldset>` disables every descendant form
control, including a `<button>`-based link, which would make it dead exactly
where a route's inline upstream is being reviewed. A real `<a>` (via
`createLink` wrapping a Mantine `Anchor`) is left untouched by `disabled` on an
ancestor `<fieldset>`, so it stays clickable in both read-only and edit views.
`RouteLinkBtn`'s own call sites are deliberately left untouched by this change.
### Dangling references and non-404 failures
A reference that resolves to nothing gets a warning icon rather than a link
— sending the user to a page that only reports the resource missing is a wasted
round trip, and a broken reference is worth stating explicitly rather than
silently. Anything that is not a 404 — in flight, a network error, a 500 —
renders nothing at all: the dashboard could not ask whether the resource
exists, which is not the same as the reference being broken, so it makes no
claim either way.
### The query carries its own retry rule
Resolving a reference means a `GET` that 404s whenever the reference is
dangling, and the app's global policy retries anything that is not a 401 three
times. A 404 here is not a failure — it is the answer "no such resource" — so
inheriting that policy meant ~8s of backoff before the warning appeared,
repeated on every typing pause in edit mode and every window refocus. The query
therefore carries its own predicate, in the same shape as the one already in
`usePluginMetadataList`, which brings that to ~1.1s on load and ~0.4s while
typing. Those are measured, and the e2e timeouts were tightened from 15s to 3s
so a return of the backoff fails the spec instead of passing slowly.
### Interaction with #3458
#3458 (open) makes the same judgement globally: it stops retrying Admin API
404s and silences their toast for reads. Once it lands, the local predicate
here becomes redundant and should collapse into it — the code says so at the
point where it would be removed. Until then, one thing this branch does **not**
fix on its own: a dangling reference still raises the gateway's `Key not found`
toast, because suppressing that would mean either threading a skip header
through the shared detail-query factory or importing #3458's global rule. That
is left to #3458 rather than duplicated here.
### Compatibility with the other in-flight PRs
This branch was verified against a local octopus merge of all five branches
currently queued against `master`: #3454, #3456, #3457, #3458 and this one. The
merge is clean on top of the other four — the only conflict in that merge is
between #3456 and #3458 themselves (both rewrite the route detail page's
`PageHeader`), and is unrelated to this change. A maintainer merging all five
in one pass will hit that conflict once, in `src/routes/routes/detail.$id.tsx`;
the resolution is to keep both sides (the test-request button from #3456 and
the id-bearing title from #3458).
**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:
- `e2e/tests/regression/form.cross-references.spec.ts` — four tests: a
resolved reference renders as a real link (asserted via `getByRole('link')`,
which fails if it ever regresses to a button) to the referenced resource's
detail page; a dangling reference shows a warning and no link; the resolved
state follows what is currently typed into the field, not the saved value; and
a consumer group reference resolves the same way on the consumer page, covering
the second call-site shape and the no-`name` fallback (consumer groups have no
`name` field). The seeded upstream is deliberately named with an ampersand so
the accessible name is pinned unescaped.
Note on the dangling fixture: it is created by force-deleting a referenced
upstream, not by writing a route that points at nothing — APISIX 3.17.0 rejects
the latter with a 400. Force-deletion is also how real deployments acquire
dangling references.
- `src/components/form/ResourceRef.test.ts` — pins each of the four
resources to its own detail query and its own route. The table that pairs them
is behind a cast that TanStack's `queryOptions()` forces, so the type system
checks the route strings but not the pairing; swapping two factories compiles
clean and this test is what catches it.
Verified: `pnpm test`, `pnpm exec tsc -b`, `pnpm lint` and a production
`pnpm build` all clean, on this branch alone and again on the five-branch
octopus merge described above. The full Playwright suite run on that merged
tree: 232 passed, 2 failed, both environmental and unrelated to this change — a
Monaco editor render race in `plugin_metadata.crud-all-fields` (finishes
suspiciously fast on the failing run, 5.6s vs its usual ~7-9s, consistent with
a race rather than a real failure) and `stream_routes.show-disabled-error`,
which restarts a compose project that does not exist on this machine. Neither
failure is specific to the merged tree or to this branch.
--
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]
