Add server catalog card actions and registration flow - #28
Conversation
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
marekdano
left a comment
There was a problem hiding this comment.
Two findings are blocking; the rest are non-blocking suggestions.
1. BLOCKED — High
File: src/pages/ServerCatalog.tsx:216
handleAdd's catch block covers both the register call and the post-success refetch(), so a refetch failure after a successful registration is caught by the same generic error handler.
Failure scenario: registerCatalogServer succeeds (server is now registered on the backend), but the subsequent await refetch() (line 216) throws — a transient network blip, aborted request, or 5xx on GET /v1/catalog.
The catch at lines 217-218 fires and shows "Unable to add this server. Try again." even though the add succeeded. The user may re-click Add, potentially double-registering, while believing the first attempt failed.
2. BLOCKED — High
File: src/pages/ServerCatalog.tsx:150
addingServerId is a single shared value for the whole page, not per-server state, so starting an Add on a second server while a first Add is still in flight clears the first server's disabled/adding indicator.
Failure scenario: User clicks Add on server A (addingServerId='A', A's button disabled + "Adding…"). Before A's request resolves, the user clicks Add on server B — addingServerId is overwritten to 'B'. Card A's isAdding (disabled={isAdding} in CatalogResults.tsx:150) becomes false even though A's registerCatalogServer call is still pending, re-enabling A's Add button and permitting a duplicate concurrent registration request for the same server. Whichever request's finally resolves first also nulls addingServerId out from under the other still-in-flight request's UI state.
3. Suggested — Medium
File: src/components/server-catalog/CatalogResults.tsx:160
For a registered/connected server, the card renders two separate controls that both open the same details dialog: the ellipsis menu's "View details" item and the always-rendered standalone "View {name}" icon button.
Failure scenario: For any server with is_registered=true, the DOM contains both an "Actions for {name}" button (lines 121-132, whose menu item at 135-141 calls onView) and a separate unconditional "View {name}" button (lines 160-171) that calls the same onView handler — confirmed by the PR's own test asserting both "Actions for Globalping" and "View Globalping" buttons exist simultaneously (src/pages/ServerCatalog.test.tsx:409-411). This is redundant, confusing UI rather than an intentional secondary affordance — the non-registered branch only shows one action.
4. Suggested — Medium
File: src/components/server-catalog/CatalogResults.tsx:134
Opening the details Dialog from a DropdownMenuItem's onSelect (without suppressing the dropdown's default close-auto-focus) can race Radix's automatic focus-return-to-trigger against the Dialog's own focus trap.
Failure scenario: User opens the "Actions for {name}" menu and activates "View details" (lines 135-141). Radix DropdownMenu's default behavior on close is to return focus to its trigger button; simultaneously the newly opened Dialog (CatalogServerDetailsDialog) tries to move focus inside itself. Depending on timing, focus can end up back on the (now covered) ellipsis trigger instead of inside the modal, breaking keyboard/screen-reader users' ability to interact with the dialog until they tab out from behind it.
5. Suggested — Low
File: src/components/server-catalog/CatalogResults.tsx:84
requiresAuth = server.auth_type !== "Open" is dead code: CatalogResults is only rendered by ServerCatalog.tsx, which pre-filters the list to auth_type === "Open" servers only, so requiresAuth is always false for any card actually shown.
Failure scenario: getOpenServers/filterOpenServers (ServerCatalog.tsx:104-128) strip out every non-Open server before CatalogResults ever sees it, so the KeyRound/Lock icon and "Auth required" CardTag (lines 93-102) can never render in production, and the fixture in ServerCatalog.test.tsx (apiKeyServer, auth_type: "API Key") is likewise filtered out before assertions run — the branch is unreachable and untested.
6. Suggested — Low
File: src/i18n/locales/en-US/mcpServer.json:27 (also es-ES, pt-BR)
The mcpServer.catalog.view translation key is now orphaned in all three locale files after the visible "View" button text was replaced with an icon-only FileText button.
Failure scenario: A repo-wide search shows no remaining mcpServer.catalog.view usage outside the locale JSON files themselves; the key (and its es-ES/pt-BR translations) is dead weight that future maintainers may mistakenly assume is still wired to UI.
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
marekdano
left a comment
There was a problem hiding this comment.
None of the issues are blocking, just suggestions for improvements
1. Registered-state override never reverts
File: src/pages/ServerCatalog.tsx:177
Category: Correctness
The optimistic registeredServerIds override on openServers is a one-way ratchet: once a server id is added, is_registered is forced to true forever, even if a later refetch's authoritative data says otherwise. Nothing in the code ever removes an id from the set.
Failure scenario: A server is registered via Add, then unregistered elsewhere (another session, or the backend gateway is later removed). The Add button stays hidden and the card shows "Connected" indefinitely — until a full page reload.
2. Add errors aren't differentiated
File: src/pages/ServerCatalog.tsx:240
Category: Correctness
handleAdd's catch-all collapses distinct backend error codes — 404 (unknown catalog id), 409 (already registered), per mcpgateway/routers/catalog.py — into one generic "Unable to add this server. Try again." message.
Failure scenario: Catalog data is stale (another session already registered the server, or it was removed from the catalog). Retrying just repeats the same failure with no differentiation and no forced refresh, until the user manually reloads.
3. Add button missing accessible name
File: src/components/server-catalog/CatalogResults.tsx:145
Category: Accessibility
The "Add"/"Adding…" button has no aria-label with the server name, unlike the sibling "View {name}" and "Actions for {name}" buttons in the same card.
Failure scenario: A screen-reader user on a catalog page with several unregistered servers hears multiple buttons all named exactly "Add" with no way to distinguish which server each belongs to.
4. Focus lost after successful Add
File: src/components/server-catalog/CatalogResults.tsx:102
Category: Accessibility
No explicit refocus target when a card's Add button unmounts after registration succeeds, unlike the View-dialog flow which restores focus via lastViewTriggerRef.
Failure scenario: A keyboard-only user tabs to a card's Add button and presses Enter. Once registration succeeds, the button is replaced by "Connected" + an Actions menu, and focus silently reverts to document.body — no test asserts focus location after a successful add.
5. Stale-data test gives false confidence
File: src/pages/ServerCatalog.test.tsx:426
Category: Test coverage
The "keeps cached catalog data visible during refreshes and refresh failures" test doesn't actually exercise a real stale-while-revalidate transition — each mockUseQuery call supplies a fresh full data object rather than simulating an in-flight/failed refetch on the same component instance.
Failure scenario: If a future change breaks the real "keep last good data during refetch" behavior (e.g. reintroducing an unconditional isLoading early-return), this test would still pass.

Summary
POST /v1/catalog/{catalog_id}/registerWhy
Catalog cards exposed read-only details but did not provide the designed Add/Connected actions or bind registration to the catalog API.
User impact
Users can now add an open catalog server directly from its card, see request progress and safe errors, and view refreshed connected state after registration.
Validation
npm run buildnpm run lint:fixnpm test(npm run test:runis not defined)npm run e2e— 180 tests passed