feat: register Quick Add picks through the catalog endpoint - #93
feat: register Quick Add picks through the catalog endpoint#93a-effort wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Registers Quick Add selections through the catalog endpoint and advances directly to component exposure.
Changes:
- Adds registration, pending, and inline error handling.
- Removes catalog-based form prefilling.
- Updates translations and automated tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/i18n/locales/pt-BR/mcpServer.json |
Updates Portuguese Quick Add messages. |
src/i18n/locales/es-ES/mcpServer.json |
Updates Spanish Quick Add messages. |
src/i18n/locales/en-US/mcpServer.json |
Updates English Quick Add messages. |
src/hooks/useMCPServerForm.ts |
Removes initial-value prefilling. |
src/hooks/useMCPServerForm.test.ts |
Updates create-mode tests. |
src/config/quickAddServers.ts |
Revises catalog-registration documentation. |
src/components/mcp-servers/QuickAddServerDialog.tsx |
Implements registration and request states. |
src/components/mcp-servers/QuickAddServerDialog.test.tsx |
Tests registration outcomes and dialog states. |
src/components/mcp-servers/MCPServerForm.tsx |
Advances registered gateways to component exposure. |
src/components/mcp-servers/MCPServerForm.test.tsx |
Tests the revised Quick Add flow. |
e2e/quick-add-server.spec.ts |
Covers registration and failure flows end-to-end. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!open) { | ||
| setSelectedId(null); | ||
| setIsConnecting(false); | ||
| setConnectError(null); |
There was a problem hiding this comment.
Fixed. handleOpenChange returns early on a close while isConnecting, so the close button, Escape, and the overlay are all blocked, and the catalog link is disabled. The dialog is also mounted only while open now, so reopening starts fresh.
marekdano
left a comment
There was a problem hiding this comment.
1. Dialog dismiss doesn't cancel in-flight registration
File: src/components/mcp-servers/QuickAddServerDialog.tsx:132
Severity: Should-fix
Dismissing the dialog while a registration is in flight doesn't cancel it, so a late success still silently advances the user to the Expose Components screen.
Failure scenario: User clicks Continue, registerCatalogServer() is in flight (isConnecting=true), then closes the dialog via the "X" button, Escape, or outside click — none of which are disabled or guarded for isConnecting since DialogContent unconditionally renders DialogPrimitive.Close and Radix's default dismiss handlers aren't intercepted. The user believes they canceled, but the in-flight promise resolves successfully and handleContinue's success path still unconditionally calls onConnected(...), which forces MCPServerForm to render ExposeComponentsForm even though the user never explicitly proceeded.
2. Missing 404 handling causes infinite retry loop
File: src/components/mcp-servers/QuickAddServerDialog.tsx:133
Severity: Should-fix
Unlike ServerCatalog.tsx's near-identical caller, handleContinue has no 404 branch for a stale/removed catalog entry, so it falls into an unrecoverable infinite retry loop.
Failure scenario: A curated QUICK_ADD_CATALOG_IDS entry is removed/renamed in the backend's mcp-catalog.yml after the dialog's fetch but before the user clicks Continue. registerCatalogServer(selectedServer.id) 404s. ServerCatalog.tsx handles this by removing the stale entry and showing a "not found" message; QuickAddServerDialog instead shows the generic "Unable to connect this server. Try again." The stale entry stays selected with no refetch, so retrying resubmits the same request and 404s again indefinitely.
3. Quick Add always creates public, teamless servers
File: src/components/mcp-servers/MCPServerForm.tsx:116
Severity: Likely blocking — confirm intent with author
Quick Add bypasses the connect form entirely, so the resulting virtual server is always created with default visibility (public) and no team, with no UI to change this.
Failure scenario: A user Quick Adds a server intending to scope the resulting virtual server to their team or keep it private. handleQuickAddConnected only calls setCreatedGateway/setQuickAddOpen(false) and never touches visibility/teamId, so ExposeComponentsForm renders with useMCPServerForm's untouched initial state (visibility='public', teamId=''). The exposed virtual server is silently created public with no team regardless of intent, and the Quick Add flow offers no control to change this before exposing components.
1dc28bf to
572426f
Compare
Selecting a server in the Quick Add dialog only prefilled the connect form and
left the user to submit it. It now registers via /v1/catalog/{id}/register and
goes straight to the detected components step, so the form is skipped.
Pending and failure state stay in the dialog. Drops the form prefill plumbing,
which no longer has a caller.
Signed-off-by: Anna Effort <anna.effort@ibm.com>
DialogContent is vertically centred, so swapping a 16px inline loader for the ~200px card grid re-centred the whole box in one frame and read as a bounce on open. Loading, error and empty now render over a skeleton grid sized from the curated id list. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Give the dialog its own visibility and team control, sent on the register body and passed to the components step. Ignore dismissal while a registration is in flight, and drop a curated entry the backend 404s on. Signed-off-by: Anna Effort <anna.effort@ibm.com>
8204ede to
e80e2c9
Compare
|
Thanks for the review! Fixed:
Notes:
Rebased onto #87, which now carries main, for #89's |
The field rendered only once the grid arrived, so the dialog grew by its height and re-centred. Render it while loading too, disabled. The error and empty states keep it hidden, since there is nothing there to scope. Signed-off-by: Anna Effort <anna.effort@ibm.com>
6f9e93c to
15c65ee
Compare
marekdano
left a comment
There was a problem hiding this comment.
🚫 Blocker — fix before merge
1. Possible duplicate registration on rapid double-click
src/components/mcp-servers/QuickAddServerDialog.tsx:150
handleContinue guards re-entrancy only via React state (isConnecting), not a synchronous ref. A fast double-click on "Continue" before the disabled attribute commits can fire two concurrent registerCatalogServer calls for the same catalog entry — one may 409, or the backend may create two gateways.
Fix: add a
useRefboolean (e.g.isSubmittingRef) set synchronously at the top ofhandleContinue, checked and returned-early on before any async call; reset it in thefinallyblock alongsidesetIsConnecting(false).
2. Specific backend error message is dropped on failure
src/components/mcp-servers/QuickAddServerDialog.tsx:179
On result.success === false, the code only reads result.message and ignores the response's dedicated error field. If the backend returns { success: false, message: '', error: 'catalog entry disabled by admin' }, the UI shows only the generic "Unable to connect this server" fallback instead of the specific reason.
Fix: change the fallback chain to
result.message || result.error || fallbackText.
💡 Nice-to-have — worth a comment, shouldn't block
3. Visibility/team picker duplicated from CatalogApiKeyDialog
src/components/mcp-servers/QuickAddServerDialog.tsx:307 (vs. src/components/server-catalog/CatalogApiKeyDialog.tsx:47-97, 165-213)
The visibility Select, "team requires a team" validation, and TeamSelect wiring are copy-pasted from CatalogApiKeyDialog.tsx (PR #89) rather than extracted into a shared hook/component. Future changes to visibility rules need to be kept in sync manually in both places.
Suggestion: file a follow-up ticket to extract a shared
useVisibilityScopehook or<VisibilityTeamPicker>component used by both dialogs. Not required for this PR.
Stacked on #87.
Selecting a server in the Quick Add dialog only prefilled the connect form and left the user to press submit, so it never reached the detected components step.
Continuenow callsregisterCatalogServer(server.id), the same/v1/catalog/{id}/registerendpoint the catalog page's Connect uses on the sameauth_type: "Open"entries, and hands the returnedserver_idtoExposeComponentsForm. The connect form is skipped entirely, so the backend owns the URL, transport, and auth details instead of the client reconstructing them.The dialog carries its own visibility and team control, following
CatalogApiKeyDialogfrom #89: private by default, with a team picker when visibility is Team and Continue blocked until a team is chosen. Both go out on the register body and are passed to the components step, so Quick Add sets its own scope rather than inheriting the connect form's defaults.Pending and failure state stay in the dialog.
Continueshows "Connecting...", the radio group, Cancel, and the catalog link disable while the request is in flight, and dismissal is ignored until it settles, since nothing aborts the request. Failures render inline without closing. Entries already carryingis_registeredandgateway_idskip the round trip. A 409 means the loaded catalog list has gone stale, so that case points at the catalog rather than retrying into the same conflict. A 404 means the entry has left the catalog, so the card is dropped and cannot be picked again.Since nothing prefills the form now, this drops
initialValuesandMCPServerFormInitialValuesfromuseMCPServerForm, along with the seeding effect andmapCatalogTransport. The dialog is mounted only while open, so a closed Quick Add no longer fetches the catalog or the caller's teams.Known: when the entry is already registered the register call is skipped, so the chosen scope reaches only the virtual server. An existing registration keeps the visibility it had.
Testing:
tsc -b, eslint, prettier, and the full vitest suite (3326 passed) are green.The e2e spec is rewritten but unrun: Playwright's chromium is not installed locally, which fails the pre-existing "Browse full catalog" case too. Its cases assert the register POST body and the expose heading.