Repository navigation
Conversation
ceb9a29 to
54fada7
Compare
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
54fada7 to
c7605dc
Compare
a-effort
left a comment
a-effort
left a comment
There was a problem hiding this comment.
Reviewed again; nice work on addressing change requests! I notice that IBM/mcp-context-forge#4684 which was created in May had gone stale and the proposed interaction for servers that need authentication was established afterwards in #130. We should use that pattern. Details:
Here's a quick demo of the desired interaction: #130 (comment) (0:23 clicking "Auth" triggers a popup for OAuth without triggering the drawer to open). After auth is complete, the status changes to active (or remains the same if it was unsuccessful as the popup communicates failure).
We should use this pattern in the drawer, also. I created a couple of mockups to show the desired changes: authorization required / auth succeeded
I also moved the button for refresh to next to the search icon and changed the styling to match that. This should persist and uses the same mechanism as the refresh overflow menu item for the server in the server table.
Lmk if there are questions / things to discuss and apologies for any confusion coming from that stale issue.
| setSelectedServerIdForDetails(id); | ||
| setDetailsInitialTab("components"); | ||
| setIsDetailsDrawerOpen(true); |
There was a problem hiding this comment.
These lines open the drawer and set its tab before triggerOAuthAuthorization runs, so the drawer opens even when the user cancels the popup or the provider never posts back. Remove them so the row chip only starts the popup. detailsInitialTab then has no writer for the components case.
| onRetryOAuthStatus={panelServer ? () => void retryOAuthStatus(panelServer.id) : undefined} | ||
| /> | ||
| <ConfirmDialog | ||
| open={isDetailsDrawerOpen && panelAttempt?.state === "failed" && panelAttempt.dialogOpen} |
There was a problem hiding this comment.
This dialog only opens when isDetailsDrawerOpen is true, and panelAttempt comes from panelServer at lines 109 to 111, which comes from selectedServerIdForDetails. Once the row click no longer opens the drawer, a failure started from a row gets no dialog. Key the dialog and the dismissed-failure state off the server being authorized instead.
| onCloseAutoFocus={(event) => { | ||
| const authorize = document.getElementById( | ||
| `oauth-authorize-${panelServer?.id}`, | ||
| ) as HTMLButtonElement | null; | ||
| const target = | ||
| authorize && !authorize.disabled | ||
| ? authorize | ||
| : document.getElementById(`mcp-server-close-${panelServer?.id}`); | ||
| if (target) { | ||
| event.preventDefault(); | ||
| target.focus(); |
There was a problem hiding this comment.
Both ids here are rendered inside the drawer, so neither exists for an attempt started from a row. Return focus to the status chip that started the attempt. The close button fallback is then unused.
| <div className="mb-5 space-y-3"> | ||
| <div className="flex items-center gap-2"> | ||
| {onAuthorize && needsAuthorization && ( | ||
| <Button | ||
| id={`oauth-authorize-${server.id}`} | ||
| variant="outline" | ||
| size="sm" | ||
| disabled={isAuthorizing} | ||
| onClick={() => void onAuthorize()} | ||
| > | ||
| {intl.formatMessage({ | ||
| id: isAuthorizing | ||
| ? "mcpServer.status.authorizing" | ||
| : "mcpServer.oauth.authorize", | ||
| })} | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
Remove this button. In the details panel, the user can click on "Authorization required" to authorize.
| {onRefresh && ( | ||
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| disabled={isRefreshing || isAuthorizing} | ||
| onClick={() => void onRefresh()} | ||
| > | ||
| {intl.formatMessage({ | ||
| id: isRefreshing | ||
| ? "mcpServer.oauth.refreshing" | ||
| : "mcpServer.oauth.refresh", | ||
| })} | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
Move refresh next to search (icon style button). Line 530 already has the container with the search button. It should stay visible and enabled regardless of OAuth state, so drop isAuthorizing from the disabled condition and move the labels out of the mcpServer.oauth.* namespace.
| {oauthAttempt?.state === "succeeded" && ( | ||
| <InlineNotification | ||
| type="success" | ||
| message={intl.formatMessage({ id: "mcpServer.oauth.success" })} |
There was a problem hiding this comment.
Remove this. The status changing to Active is the success signal. mcpServer.oauth.success can be deleted from each locale file, nothing else uses it. The succeeded attempt state then has no reader; it is declared at src/api/oauth.ts:108 and set at Servers.tsx:304. It still clears a prior failure, so it can stay.
| <ServerStatusIndicator | ||
| server={server} | ||
| oauthStatus={oauthStatus} | ||
| onRetry={onRetryOAuthStatus} | ||
| authorizing={isAuthorizing} |
There was a problem hiding this comment.
Pass onAuthorize here. ServerStatusIndicator renders the chip as the authorize trigger when it has that prop and the status needs authorization, which is the pattern from #130.
| {needsAuthorization && onAuthorize && ( | ||
| <Button | ||
| variant="outline" | ||
| size="xs" | ||
| disabled={isAuthorizing} | ||
| aria-label={intl.formatMessage( | ||
| { id: "mcpServer.status.authorizeTrigger" }, | ||
| { name: server.name }, | ||
| )} | ||
| onClick={() => void onAuthorize()} | ||
| > | ||
| {intl.formatMessage({ | ||
| id: isAuthorizing | ||
| ? "mcpServer.status.authorizing" | ||
| : "mcpServer.oauth.authorize", | ||
| })} | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
Remove this button once the indicator above takes onAuthorize. It also duplicates the label: line 688 puts the indicator into its authorizing state and line 703 sets this button to the same text, so both read Authorizing at once. The indicator tracks authorizing itself, so line 688 can go too.
Closing the OAuth popup without completing authorization is a deliberate
user action, not a failure, so ServerCatalog no longer reports it.
Previously both `handleAuthorize` (page-level notice above the grid) and
`handleOAuthSubmit` (notice inside the Add-server dialog) passed the raw
`OAuthCancelledError` message ("OAuth authorization was cancelled",
untranslated) straight into their notification state.
Both catch blocks now return early on `OAuthCancelledError` instead of
notifying, mirroring the MCP Servers page (Servers.tsx), which already
discards the cancellation silently. Genuine authorization failures keep
surfacing their reason, and the dialog stays open for a retry in both
cases.
Tests: the cancelled-authorization test now uses `OAuthCancelledError`
and asserts no cancellation notice appears anywhere (previously it
rejected with a plain `Error` carrying the same text, covering cancel
and failure identically); a new dialog test covers the real-failure path
(provider reason still shown), and a new page-level test covers the
Authorize menu action staying quiet on cancellation.
Fixes IBM/mcp-context-forge#7303 (the catalog UI lives in this repo).
Note: surfacing the provider's denial reason via `OAuthAuthorizationError`
(the remaining suggestion in that issue) lands naturally after contextforge-org#199 adds
the class to `src/api/servers.ts`; this PR intentionally does not
duplicate it.
Verification: `vitest run src/pages/ServerCatalog.test.tsx` 78/78 green;
`tsc -b tsconfig.app.json` clean (after `orval` generation); eslint and
prettier clean on both changed files.
Assisted-by: AI (Claude via WorkBuddy) — diff reviewed and verified by
the human contributor before submission.
Signed-off-by: ShaoLOWu <3034261766@qq.com>
Summary
OAuth authorization on the MCP Servers page opens the server's Components view, shows success inline, and reloads tools, resources, and prompts. Failures display the provider's bounded, validated plain-text reason in a dialog with retry. Dismissal preserves the shared Authorization required or Authorization expired status with an adjacent authorization action.
Fixes IBM/mcp-context-forge#4684.
Validation
npm run buildandnpm run lintpassed.git diff --checkpassed for the changes.e2e/tools.spec.tsandsrc/utils/errors.ts.Manual verification
On an existing OAuth authorization-code server without a valid caller token, deny consent, dismiss the failure dialog, retry from Authorization required, and complete authorization. Confirm inline success and freshly loaded components, then use Refresh components without reopening OAuth. Check that expired tokens retain Authorization expired, valid tokens do not offer Authorize, Escape dismisses only the dialog, and closing a pending popup produces no failure dialog.