fix: surface failed component list loads in expose components form - #106
fix: surface failed component list loads in expose components form#1060717lee wants to merge 3 commits into
Conversation
A failed /tools, /resources, or /prompts request rendered identically to a successful empty one: the section showed a zero count with no error. Read the useQuery error per section, show the count line as "Failed to load ..." with an error notification and a retry action, and keep real counts (including legitimate zeros) for sections whose request succeeded. Signed-off-by: Li Fengmin <2080291162@qq.com>
gcgoncalves
left a comment
There was a problem hiding this comment.
Thanks @0717lee for this PR, the error handling is a nice improvement to the form. There are a couple suggestions for the proposed implementation:
First, it'd be nice to improve i18n (see comment below). Also, when a user clicks Retry for the tools section, toolsLoading becomes true. Then, the entire form is replaced by a spinner, including the resources and prompts sections, that are still valid.
My suggestion is using the individual loading states to add guards to each section:
{toolsLoading ? (
<span className="text-sm text-muted-foreground">
{intl.formatMessage({ id: "common.loading" })}
</span>
) : toolsError ? (
intl.formatMessage({ id: "gateways.exposeComponents.error.tools" })
) : (
intl.formatMessage({ id: "gateways.card.toolCount" }, { count: toolCount })
)}| message={ | ||
| toolsError.message | ||
| ? `Failed to load tools: ${toolsError.message}` | ||
| : "Failed to load tools" |
There was a problem hiding this comment.
All the "Failed to load tools" instances are not internationalised. Please add the appropriate keys to the i18n files and invoke them here and on line 365.
- Add gateways.exposeComponents.error.{tools,resources,prompts}(WithDetail)
and exposeComponents.promptCount keys to en-US, pt-BR, and es-ES; reuse
the existing gateways.card.*Count keys for tools and resources.
- Replace the form-level spinner on refetch with per-section count-row
states (loading / error / count); the full-form spinner now only covers
the initial load before any section has resolved, so retrying one failed
section no longer unmounts the healthy ones.
Signed-off-by: Li Fengmin <2080291162@qq.com>
|
Thanks for the review @gcgoncalves — both points addressed in 79cdee2:
Added a regression test ("should keep healthy sections mounted while a failed section retries") that pins this: during a hanging tools retry, the other sections stay mounted and no form-level spinner appears. Full vitest (3324 passed), |
| : intl.formatMessage({ id: "gateways.exposeComponents.error.tools" }) | ||
| } | ||
| action={{ | ||
| label: "Retry", |
There was a problem hiding this comment.
i18n this label as well, please. Key: common.button.retry
Signed-off-by: Li Fengmin <2080291162@qq.com>
Closes #6549
Summary
In the expose step, the three component lists are loaded with
useQuerybut onlydata,isLoading, andrefetchare destructured —erroris never read. A failed/tools,/resources, or/promptsrequest therefore renders identically to a successful empty one: the section shows "0 tools" / "0 resources" / "0 prompt templates" and invites pressing "Expose components" on what looks like a server offering nothing.STATUS_TONE_CLASS) instead of a zero count, and the section renders an errorInlineNotificationwith the API error message and a Retry action that refetches just that list.Unit tests cover the failed/empty distinction, per-section independence (some sections fail while others keep their counts), and recovery through Retry.