Add logo_uri to OAuth client metadata for DCR branding#1554
Add logo_uri to OAuth client metadata for DCR branding#1554ignaciojimenezr wants to merge 3 commits intoMCPJam:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughAdds a new Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts (1)
140-145: Extract the branding URL into a shared constant.This literal now exists in both OAuth providers. A single source of truth will keep the DCR metadata aligned when branding changes again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts` around lines 140 - 145, Extract the repeated branding URL literal into a shared exported constant (e.g., BRANDING_LOGO_URL or MCP_BRANDING_URL) in a common module and replace the hard-coded value in the clientMetadata getter (logo_uri) with that constant; update the other OAuth provider to import and use the same constant so both providers reference a single source of truth for the branding URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 140-145: Extract the repeated branding URL literal into a shared
exported constant (e.g., BRANDING_LOGO_URL or MCP_BRANDING_URL) in a common
module and replace the hard-coded value in the clientMetadata getter (logo_uri)
with that constant; update the other OAuth provider to import and use the same
constant so both providers reference a single source of truth for the branding
URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 815f46ae-92f9-480c-a79f-9db4a5a5f43f
📒 Files selected for processing (3)
mcpjam-inspector/client/src/lib/oauth/__tests__/client-metadata.test.tsmcpjam-inspector/client/src/lib/oauth/debug-oauth-provider.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/lib/oauth/__tests__/client-metadata.test.ts (1)
8-15: Prefer the shared client-test mock presets here.This test hand-rolls its mocks instead of using the shared presets, which makes it easier for OAuth tests to drift from the rest of the client suite. If a needed preset is missing, extending the shared mock layer would still be the better long-term path.
As per coding guidelines,
mcpjam-inspector/client/**/__tests__/*.test.{ts,tsx}: Use mock presets fromclient/src/test/mocks/includingmcpApiPresetsandstorePresetsin client tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/__tests__/client-metadata.test.ts` around lines 8 - 15, Replace the two hand-rolled mocks for authFetch and generateRandomString with the shared test mock presets: import and apply the common presets (including mcpApiPresets and storePresets) from the client test mocks module and use them in this test instead of vi.mock("@/lib/session-token", ...) and vi.mock("../state-machines/shared/helpers", ...); ensure the shared preset provides a stub for authFetch and either includes generateRandomString or extend the shared presets to provide generateRandomString => "mock-random-string" so the test no longer defines its own mocks.mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts (1)
140-149: Add explicit return type annotation toclientMetadatafor enhanced clarity and compiler-enforced SDK boundaries.This getter should declare its return type as
OAuthClientMetadatafrom@modelcontextprotocol/sdk/shared/auth.js, aligning with the pattern used inDebugMCPOAuthClientProviderand ensuring future metadata modifications remain type-safe.Proposed change
import { auth, OAuthClientProvider, } from "@modelcontextprotocol/sdk/client/auth.js"; +import type { OAuthClientMetadata } from "@modelcontextprotocol/sdk/shared/auth.js"; ... - get clientMetadata() { + get clientMetadata(): OAuthClientMetadata { return { client_name: `MCPJam - ${this.serverName}`, client_uri: "https://github.com/mcpjam/inspector", logo_uri: "https://www.mcpjam.com/mcp_jam_2row.png",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts` around lines 140 - 149, The clientMetadata getter lacks an explicit return type; update the getter signature on the class (clientMetadata) to declare its return type as OAuthClientMetadata and add an import for OAuthClientMetadata from `@modelcontextprotocol/sdk/shared/auth.js` so the returned object (client_name, client_uri, logo_uri, redirect_uris, grant_types, response_types, token_endpoint_auth_method) is type-checked consistently with DebugMCPOAuthClientProvider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/lib/oauth/__tests__/client-metadata.test.ts`:
- Around line 8-15: Replace the two hand-rolled mocks for authFetch and
generateRandomString with the shared test mock presets: import and apply the
common presets (including mcpApiPresets and storePresets) from the client test
mocks module and use them in this test instead of vi.mock("@/lib/session-token",
...) and vi.mock("../state-machines/shared/helpers", ...); ensure the shared
preset provides a stub for authFetch and either includes generateRandomString or
extend the shared presets to provide generateRandomString =>
"mock-random-string" so the test no longer defines its own mocks.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 140-149: The clientMetadata getter lacks an explicit return type;
update the getter signature on the class (clientMetadata) to declare its return
type as OAuthClientMetadata and add an import for OAuthClientMetadata from
`@modelcontextprotocol/sdk/shared/auth.js` so the returned object (client_name,
client_uri, logo_uri, redirect_uris, grant_types, response_types,
token_endpoint_auth_method) is type-checked consistently with
DebugMCPOAuthClientProvider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eae9a356-0bd6-4c5e-8899-e8174d4d6a28
⛔ Files ignored due to path filters (1)
mcpjam-inspector/client/public/mcp_jam_2row.pngis excluded by!**/*.png
📒 Files selected for processing (3)
mcpjam-inspector/client/src/lib/oauth/__tests__/client-metadata.test.tsmcpjam-inspector/client/src/lib/oauth/debug-oauth-provider.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/lib/oauth/debug-oauth-provider.ts
CIMD pr is in web app repo (related to #1552)
Include logo_uri in MCPOAuthProvider and DebugMCPOAuthClientProvider
client metadata so authorization servers display MCPJam branding on
consent screens during Dynamic Client Registration (DCR).
Tested:
confirmed POST to /register includes logo_uri in request body