fix: e2e ui tests for mcp registry#4558
Merged
Merged
Conversation
This was referenced Jun 19, 2026
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe MCP registry OAuth e2e tests are refactored to use a centralized ChangesMCP Registry OAuth E2E Test Refactor
Sequence Diagram(s)sequenceDiagram
participant Spec as Test Spec
participant Page as MCPRegistryPage
participant Popup as OAuth Popup
participant Status as Status Endpoint
participant Complete as Complete Endpoint
Spec->>Page: createOAuthClient(config)
Page-->>Spec: { authorize_url, status_url, complete_url, ... }
Spec->>Popup: open authorize_url, fill login form, submit
loop poll until status == authorized
Spec->>Status: GET status_url
Status-->>Spec: { status }
end
Spec->>Complete: POST complete_url
Complete-->>Spec: 200 OK
Spec->>Page: goto()
Spec->>Page: assertClientExists()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
18 tasks
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
Confidence Score: 5/5Safe to merge — changes are confined to e2e test infrastructure with no production code touched. The refactor correctly replaces fragile popup-event interception with a well-guarded polling loop. The complete-oauth fallback URL correctly uses oauth_config_id (confirmed against the Go handler and the existing UI mcpApi.ts). The only noteworthy code issue is that createClient's pendingOAuth branch discards the locator returned by waitForOAuthAuthorizerAction without clicking it, but OAuth tests no longer go through that path, so it has no effect on the current suite. The pendingOAuth branch in createClient inside mcp-registry.page.ts is worth cleaning up to avoid future confusion, but it is not reachable by any test in this PR. Important Files Changed
Reviews (2): Last reviewed commit: "fix: e2e ui tests for mcp registry" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/features/mcp-registry/mcp-registry.spec.ts (2)
14-14: ⚡ Quick winReplace
anytypes with proper Playwright types for type safety.The
pageparameter usesanyfor bothcontext()return andrequest, which loses type safety and violates the TypeScript strictness guideline. Use Playwright'sPagetype directly.♻️ Suggested fix
+import { Page } from '`@playwright/test`' + -async function completeOAuthFlow(page: { context: () => any; request: any }, flow: { +async function completeOAuthFlow(page: Page, flow: { authorize_url: string oauth_config_id: string complete_url?: string status_url?: string }) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/features/mcp-registry/mcp-registry.spec.ts` at line 14, The completeOAuthFlow function parameter page uses any types for both context() and request properties, losing type safety. Replace the page parameter type annotation with Playwright's proper Page type instead of the current object definition with any values. Make sure to import the Page type from the Playwright testing library if it is not already imported at the top of the file.Source: Coding guidelines
39-39: ⚡ Quick winAdd a descriptive failure message to the polling timeout assertion.
When the polling loop exhausts all 30 attempts without reaching
authorized, the assertionexpect(authorized).toBe(true)fails with a generic message. Adding context helps diagnose flaky test failures.♻️ Suggested fix
- expect(authorized).toBe(true) + expect(authorized, `OAuth status did not become 'authorized' after polling ${statusUrl} for 15s`).toBe(true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/features/mcp-registry/mcp-registry.spec.ts` at line 39, The expect assertion for authorized in the polling loop lacks a descriptive failure message, making it difficult to diagnose why the test fails. Modify the expect(authorized).toBe(true) statement to include a custom failure message as the second parameter that provides context about what was being waited for and how many polling attempts were made. This will help clarify the root cause when the 30-attempt polling timeout is exhausted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/features/mcp-registry/pages/mcp-registry.page.ts`:
- Around line 514-519: The return type declares mcp_client_id as a required
string field, but the validation check only verifies status, authorize_url, and
oauth_config_id properties. Add mcp_client_id to the existing condition
validation alongside the other required fields to ensure the API response
actually contains this required property before returning the body, preventing
the caller from receiving undefined values despite the type contract.
---
Nitpick comments:
In `@tests/e2e/features/mcp-registry/mcp-registry.spec.ts`:
- Line 14: The completeOAuthFlow function parameter page uses any types for both
context() and request properties, losing type safety. Replace the page parameter
type annotation with Playwright's proper Page type instead of the current object
definition with any values. Make sure to import the Page type from the
Playwright testing library if it is not already imported at the top of the file.
- Line 39: The expect assertion for authorized in the polling loop lacks a
descriptive failure message, making it difficult to diagnose why the test fails.
Modify the expect(authorized).toBe(true) statement to include a custom failure
message as the second parameter that provides context about what was being
waited for and how many polling attempts were made. This will help clarify the
root cause when the 30-attempt polling timeout is exhausted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dae01e9b-8447-45ab-a520-59742c7c188e
📒 Files selected for processing (2)
tests/e2e/features/mcp-registry/mcp-registry.spec.tstests/e2e/features/mcp-registry/pages/mcp-registry.page.ts
sammaji
force-pushed
the
06-19-fix_e2e_ui_tests_for_mcp_registry
branch
from
696b27a to
bd8e1d7
Compare
sammaji
force-pushed
the
06-18-fix_e2e_ui_tests_for_model_limts
branch
from
eee4550 to
2ebd0ab
Compare
Contributor
Merge activity
|
akshaydeo
changed the base branch from
06-18-fix_e2e_ui_tests_for_model_limts
to
graphite-base/4558
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refactors the OAuth e2e test flow to use a direct API-based approach instead of relying on browser popup events. This resolves flakiness caused by waiting for popup windows to open and close during OAuth authorization in tests.
Changes
completeOAuthFlowhelper that navigates to the authorization URL in a new page, completes the login form, then polls the OAuth status endpoint untilauthorizedand calls the complete-oauth endpoint directly — bypassing the need to intercept popup events.createOAuthClientmethod toMCPRegistryPagethat intercepts thepending_oauthAPI response and returns theauthorize_url,oauth_config_id, and related fields for use incompleteOAuthFlow.selectAuthTypeto reflect the new UI split between auth type (e.g.,oauth) and auth scope (e.g.,sharedvsper_user).per_user_oauthis now expressed as auth typeoauth+ scopeper_user.selectAuthScopemethod to handle the newauth-scope-selectdropdown.expandOAuthAdvancedIfCollapsedto expand the collapsible OAuth advanced section before interacting with fields like client ID and secret.data-testidattributes with placeholder-based selectors as fallbacks.viewClientDetailsto open the actions menu and click "Edit" rather than clicking the row directly.Type of change
Affected areas
How to test
Run the MCP registry e2e tests with the OAuth demo server running:
MCP_SSE_HEADERS=1 npx playwright test tests/e2e/features/mcp-registry/mcp-registry.spec.tsVerify that the OAuth and per-user OAuth client creation tests complete without flakiness, and that the created clients appear as connected in the registry table.
Screenshots/Recordings
N/A — no UI visual changes.
Breaking changes
Related issues
N/A
Security considerations
No new auth flows or secrets handling introduced. The test helper uses the existing OAuth demo server and status/complete endpoints already present in the application.
Checklist
docs/contributing/README.mdand followed the guidelines