feat: show catalog add failure error on the card by a-effort · Pull Request #153 · contextforge-org/contextforge-web-ui

a-effort · 2026-09-24T01:35:35Z

Summary:
Add failure errors (for open MCP servers) now appear on the card that failed with more info available in a popover.

153.mp4

This fixes the current behavior where errors push down the cards and may be far from the card they relate to (ux issue).

Closes IBM/mcp-context-forge#6463

Notes:

The presentational part of ServerStatusIndicator moves into ui/status-indicator, so the card reuses the icon, label and popover without inheriting the server availability model.

Some outcomes stay on the page stack, either because no card is left to carry them or because the message belongs elsewhere: a 404 removes the card, a 409 is reported as success, and the API key and OAuth flows keep the notification inside their dialog.

The announcement uses a bare aria-live region rather than role="status", since CatalogResults already has one for the result count and a second breaks its accessible name lookups.

The ui/status-indicator extraction this builds on is under review in [#160](#160). Once #160 merges, I will rebase this onto it, and this PR will contain only the catalog feature.

Copilot

Copilot review overview

🟡 Changes recommended

The add-error live region retains stale content and cannot reannounce an identical retry failure.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Clear stale live-region error message when retry starts · New

What changed in this PR

Adds card-level catalog registration errors and introduces reusable MCP server/OAuth status presentation.

Changes:

Displays add failures on the affected catalog card with accessible announcements.
Adds OAuth-aware server statuses and authorization actions.
Adds localized strings and comprehensive component/API tests.

File	Description
`src/types/server.ts`	Adds status and OAuth response types.
`src/pages/Servers.tsx`	Loads OAuth statuses and handles authorization.
`src/pages/Servers.test.tsx`	Tests authorization status refreshes.
`src/pages/ServerCatalog.tsx`	Adds card-level registration errors.
`src/pages/ServerCatalog.test.tsx`	Tests card errors and announcements.
`src/lib/serverStatus.ts`	Centralizes server availability classification.
`src/lib/serverStatus.test.ts`	Tests availability states.
`src/i18n/locales/pt-BR/mcpServer.json`	Adds Portuguese status messages.
`src/i18n/locales/es-ES/mcpServer.json`	Adds Spanish status messages.
`src/i18n/locales/en-US/mcpServer.json`	Adds English status messages.
`src/components/ui/status-indicator.tsx`	Adds reusable status indicator.
`src/components/ui/status-indicator.test.tsx`	Tests indicator behavior.
`src/components/servers/ServerStatusIndicator.tsx`	Adds OAuth-aware server status UI.
`src/components/servers/ServerStatusIndicator.test.tsx`	Tests server status interactions.
`src/components/servers/ServerStatusDetail.tsx`	Displays status details and errors.
`src/components/servers/ServerStatusBadge.tsx`	Removes superseded status badge.
`src/components/servers/ServerStatusBadge.test.tsx`	Removes obsolete badge tests.
`src/components/servers/ServersTable.tsx`	Integrates the new status indicator.
`src/components/servers/ServersTable.test.tsx`	Updates status table tests.
`src/components/server-catalog/CatalogResults.tsx`	Renders card error indicators.
`src/api/servers.ts`	Adds batched OAuth status retrieval.
`src/api/servers.test.ts`	Tests OAuth status batching.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Extracts the presentational shell of ServerStatusIndicator so the catalog card can reuse the icon, muted label and popover without inheriting the server availability model. Open-auth add failures now render an error indicator in the card footer next to Add, with the reason in a popover, instead of a notification above the grid that pushed the cards down. The failures that have no card left to carry them, and every other action outcome, still use the page stack. Signed-off-by: Anna Effort <anna.effort@ibm.com>

Radix gives PopoverContent role="dialog" without a name, so the shared indicator's popover announced as an unnamed dialog. Adds contentAriaLabel alongside the existing triggerAriaLabel and supplies it from both callers. The trigger's string is not reused: it ends in "Show details", which reads as an instruction to open what is already open and repeats the status text. Signed-off-by: Anna Effort <anna.effort@ibm.com>

The label uses the foreground token instead of muted, and the indicator sits 8px further from the view button. The muted label stays the default on StatusIndicator, so the servers page and the source picker are unchanged. Signed-off-by: Anna Effort <anna.effort@ibm.com>

The live region kept the previous message, so a second identical failure set the same string, produced no DOM change and announced nothing. Emptying it when the retry starts also drops the stale error once an add succeeds. Signed-off-by: Anna Effort <anna.effort@ibm.com>

Copilot

Copilot review overview

🟡 Changes recommended

The reusable component permits interactive popovers with unnamed dialogs, creating an accessibility gap.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Require accessible labels for interactive popover indicators · New

Resolved since last review (1)

Clear stale live-region error message when retry starts

The two reporters were trailing positional params, so wanting onAddFailure alone meant passing undefined for reportNotification. Signed-off-by: Anna Effort <anna.effort@ibm.com>

The card error tests all went through a thrown request. A resolved { success: false } response takes a different branch and is the only one that shows the backend's own message. Signed-off-by: Anna Effort <anna.effort@ibm.com>

marekdano

Findings

🔴 Critical — fix before merge

1. Screen-reader announcement isn't scoped per server

File: src/pages/ServerCatalog.tsx:498

addErrorAnnouncement is a single page-wide string shared by every server. If two Add failures happen in quick succession (or an open-auth failure overlaps another dialog submit, since clearAddError runs unconditionally on every flow), the later call can overwrite/blank the string before assistive tech announces the earlier one — React batching means the intermediate value may never even hit the DOM. The first failure is silently never announced, though the card still shows it visually.

2. Stale error can resurface on card remount

File: src/components/server-catalog/CatalogResults.tsx:449

addErrors lives in parent state and is only cleared by a fresh add attempt on that
exact server id. If server A's add fails, then a search/filter unmounts and later remounts its card (e.g. a filter round-trip), the old error badge reappears even though nothing changed and no new attempt was made.

3. Announcement drops the actual failure reason

File: src/pages/ServerCatalog.tsx:500

The new aria-live text is just "Error adding {name}" — the previous role="alert"
notification spoke the real reason (e.g. "Remote server error..."). Screen-reader users
now have to separately find and activate a popover to learn why: a regression at the
moment of failure.

🟡 Medium — cleanup, worth doing but not blocking

4. New parallel error-reporting mechanism instead of extending the existing one

File: src/pages/ServerCatalog.tsx:79

RegistrationReporting.onAddFailure + addErrors + addErrorAnnouncement duplicates what could have been a target option on the existing RegistrationNotification stack. Forces two unrelated call sites (handleApiKeySubmit, handleOAuthSubmit) to wrap an unchanged callback just to match the new shape, and a call site that forgets onAddFailure silently falls back to the old stack with no compile-time signal.

5. Duplicated Tailwind class strings

File: src/components/servers/ServerStatusIndicator.tsx:65

The authorize branch now hardcodes its own copy of the layout/trigger classes that used to be shared with the Popover branch (moved into status-indicator.tsx). Future styling changes will silently miss this branch.

6. `addErrors` hand-rolls a pattern `usePendingIds` already generalizes

File: src/pages/ServerCatalog.tsx:413

Same file already uses usePendingIds three times for other per-server state; this is a fourth hand-written spread/delete implementation of the same idea.

Announce the reason alongside the name, and leave the announcement alone when a different server starts an add, whose failure is still on its card. Signed-off-by: Anna Effort <anna.effort@ibm.com>

a-effort · 2026-09-25T18:57:22Z

Thanks for reviewing @marekdano! Changes added in f790586. Details:

From LLM-assisted analysis of the feedback, the recommendation is to move forward with these changes:

Feedback item 3: implemented.

Agreed, this was a regression against the notification it replaces. The announcement now interpolates the reason alongside the name, across the three locales.

Feedback item 5: implemented.

Already covered on #160 by cd71a82.

Feedback item 1: partly implemented.

clearAddError ran at the top of every add flow, so an add on server B cleared server A's announcement while A's card still showed the error. It now returns early for a server with no error, backed by a ref as usePendingIds does in the same file, with a test that fails without the change. The batching case is not covered: separate clicks are separate render cycles and each announces, it needs more than one failure resolving in the same tick, and an aria-atomic region holds one message regardless.

From LLM-assisted analysis of the feedback, the recommendation is to not move forward with these changes:

Feedback item 2: not implemented.

The state lives in the parent so it survives list reconciliation, and after a filter round trip the error is still accurate: the last add for that server failed and nothing has changed since. There is something underneath this worth settling though. The card error has no dismiss path, where the InlineNotification it replaces passes onDismiss at ServerCatalog.tsx:1274 and restores focus on dismiss. The badge clears only on a retry, a successful add, or leaving the catalog page. That lifetime follows from where the state lives rather than from a decision, so I have filed it as IBM/mcp-context-forge#6997 rather than patching it here.

Feedback item 4: not implemented.

The seam is deliberate. Card and notification differ in presentation, not just destination, so a target option would mean rendering an InlineNotification in a card footer. On a call site forgetting onAddFailure, the fallback is the contract rather than an oversight: only open auth add failures route to a card, and everything else belongs on the stack.

Feedback item 6: not implemented.

usePendingIds is a Set<string> with a re-entrancy guard in begin. addErrors is id to message, and a Set cannot hold the message.

One thing to flag: #160 and #153 will collide. Both add src/components/ui/status-indicator.tsx and both edit ServerStatusIndicator.tsx, and they have diverged. #160 has since removed labelClassName and the interactive prop, and this branch still has both, with the catalog card passing labelClassName at CatalogResults.tsx:291. Both report mergeable against main, so this only bites on the second merge. Landing #160 first as the extraction, then rebasing this onto it and re-adding labelClassName, looks cleanest, or the label tone could fold into the #6972 footer work.

Lmk if I'm missing something, though pls! 🙌

Closing the popover clears the error, so the badge does not need a dismiss control in a card footer with no room for one. Signed-off-by: Anna Effort <anna.effort@ibm.com>

Closing the popover unmounts the trigger focus returns to, dropping it on the body. Focus moves to Add, and only where it was otherwise lost. Signed-off-by: Anna Effort <anna.effort@ibm.com>

marekdano

The PR looks good now!

LGTM 🚀

vishu-bh

LGTM 🚀

#153 landed its own copy of the status indicator shell, so the two extractions collided. Keeps main's labelClassName, size and onOpenChange alongside the shared trigger classes, the icon export and the popover name fallback from this branch, and carries forward the removal of the unused interactive prop. Signed-off-by: Anna Effort <anna.effort@ibm.com>

Main carries #130 squashed and #153's refactor of ServerStatusIndicator onto the shared status-indicator, so those files take main's version. Servers.tsx keeps main's inline OAuth status block rather than this branch's hook, which #150 replaces. Signed-off-by: Anna Effort <anna.effort@ibm.com>

a-effort requested a balanced review from Copilot September 24, 2026 16:36

Copilot started reviewing on behalf of a-effort September 24, 2026 16:37 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

Comment thread src/pages/ServerCatalog.tsx

a-effort force-pushed the 6463-catalog-card-add-error branch from 5e4c2bb to 0958e67 Compare September 24, 2026 16:48

a-effort mentioned this pull request Sep 24, 2026

feat: virtual server add source step #131

Open

a-effort added 2 commits September 24, 2026 13:03

a-effort marked this pull request as ready for review September 24, 2026 21:25

This was referenced Sep 25, 2026

refactor: make status indicator presentation reusable #160

Open

fix(ui): add caller-scoped OAuth status handling #150

Open

a-effort changed the title ~~6463 catalog card add error~~ feat: show catalog add failures on the card that failed Sep 25, 2026

a-effort changed the title ~~feat: show catalog add failures on the card that failed~~ feat: show catalog add failure error on the card Sep 25, 2026

a-effort self-assigned this Sep 25, 2026

a-effort requested a balanced review from Copilot September 25, 2026 05:47

Copilot started reviewing on behalf of a-effort September 25, 2026 05:48 View session

Copilot AI reviewed Sep 25, 2026

View reviewed changes

Comment thread src/components/ui/status-indicator.tsx

a-effort added 2 commits September 24, 2026 23:31

refactor: pass registration reporting as one options object

158a0e1

The two reporters were trailing positional params, so wanting onAddFailure alone meant passing undefined for reportNotification. Signed-off-by: Anna Effort <anna.effort@ibm.com>

test: cover the rejected-response path to the card error

be3e8d4

The card error tests all went through a thrown request. A resolved { success: false } response takes a different branch and is the only one that shows the backend's own message. Signed-off-by: Anna Effort <anna.effort@ibm.com>

a-effort requested review from gcgoncalves, marekdano and vishu-bh September 25, 2026 06:46

marekdano requested changes Sep 25, 2026

View reviewed changes

fix: scope the catalog add failure announcement

f790586

Announce the reason alongside the name, and leave the announcement alone when a different server starts an add, whose failure is still on its card. Signed-off-by: Anna Effort <anna.effort@ibm.com>

a-effort mentioned this pull request Sep 25, 2026

Decide how long a catalog card add error stays on the card IBM/mcp-context-forge#6997

Closed

a-effort added 2 commits September 25, 2026 13:03

feat: retire the catalog add error once its reason is read

96863b0

Closing the popover clears the error, so the badge does not need a dismiss control in a card footer with no room for one. Signed-off-by: Anna Effort <anna.effort@ibm.com>

fix: keep focus on the card when the add error retires

4503e37

Closing the popover unmounts the trigger focus returns to, dropping it on the body. Focus moves to Add, and only where it was otherwise lost. Signed-off-by: Anna Effort <anna.effort@ibm.com>

marekdano approved these changes Sep 28, 2026

View reviewed changes

vishu-bh approved these changes Sep 28, 2026

View reviewed changes

vishu-bh merged commit 6623520 into main Sep 28, 2026
5 checks passed

vishu-bh deleted the 6463-catalog-card-add-error branch September 28, 2026 13:51

a-effort mentioned this pull request Sep 28, 2026

feat: wire SSO token refresh into sessionAuth #163

Open

Navigation Menu

feat: show catalog add failure error on the card - #153

feat: show catalog add failure error on the card#153
vishu-bh merged 9 commits into
mainfrom
6463-catalog-card-add-error

a-effort commented Sep 24, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

marekdano left a comment

Uh oh!

a-effort commented Sep 25, 2026 •

edited

Loading

Uh oh!

marekdano left a comment

Uh oh!

vishu-bh left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

a-effort commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

marekdano left a comment

Choose a reason for hiding this comment

Findings

🔴 Critical — fix before merge

1. Screen-reader announcement isn't scoped per server

2. Stale error can resurface on card remount

3. Announcement drops the actual failure reason

🟡 Medium — cleanup, worth doing but not blocking

4. New parallel error-reporting mechanism instead of extending the existing one

5. Duplicated Tailwind class strings

6. addErrors hand-rolls a pattern usePendingIds already generalizes

Uh oh!

a-effort commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marekdano left a comment

Choose a reason for hiding this comment

Uh oh!

vishu-bh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

a-effort commented Sep 24, 2026 •

edited

Loading

6. `addErrors` hand-rolls a pattern `usePendingIds` already generalizes

a-effort commented Sep 25, 2026 •

edited

Loading