feat: Add MCP server deployments list page#6907
feat: Add MCP server deployments list page#6907YuliaKrimerman wants to merge 3 commits intoopendatahub-io:mainfrom
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (19)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
| }, | ||
| ); | ||
|
|
||
| export const deleteMcpDeployment = |
There was a problem hiding this comment.
this is not used anywhere - should be cleaned up/edited
| @@ -0,0 +1,60 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
McpDeploymentsWrapper is same as McpCatalogWrapper - is this intentional ?
| required: [SupportedArea.MCP_CATALOG], | ||
| }, | ||
| properties: { | ||
| path: '/ai-hub/mcp-deployments/*', |
There was a problem hiding this comment.
This will conflict with #6771 based on whichever goes in first - FYI no action needed until we see merge conflicts
| }; | ||
|
|
||
| const McpDeploymentsWrapper: React.FC = () => { | ||
| const modularArchConfig: ModularArchConfig = { |
There was a problem hiding this comment.
The highest priority fix is adding useFetchDscStatus and passing mandatoryNamespace to the config — without it, the namespace selector behavior will be incorrect in production environments. Is issing this intentional ?
There was a problem hiding this comment.
Nope, it was not intentional — it was a local testing ovveride to actually be able to see the table. Restored useFetchDscStatus and mandatoryNamespace to match the McpCatalogWrapper pattern.
| import { McpDeployment, McpDeploymentPhase } from '~/app/mcpDeploymentTypes'; | ||
| import { getConnectionUrl, getServerDisplayName, getStatusInfo } from '../utils'; | ||
|
|
||
| const createMockDeployment = (overrides: Partial<McpDeployment> = {}): McpDeployment => ({ |
There was a problem hiding this comment.
This is duplicated: packages/model-registry/upstream/frontend/src/app/pages/mcpDeployments/__tests__/McpDeploymentsTableRow.spec.tsx
manaswinidas
left a comment
manaswinidas
left a comment
There was a problem hiding this comment.
Flagging tautological / repetitive tests — see inline comments.
| it('should render the table element', () => { | ||
| render( | ||
| <McpDeploymentsTable | ||
| deployments={mockDeployments} | ||
| toolbarContent={<div>toolbar</div>} | ||
| onClearFilters={onClearFilters} | ||
| onDeleteClick={onDeleteClick} | ||
| />, | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| expect(screen.getByTestId('mcp-deployments-table')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Redundant — if "should render all deployment rows" passes (rows render), the table element necessarily exists. This only verifies PatternFly renders a <table>, not any application logic.
| it('should show empty table view when no deployments match filter', () => { | ||
| render( | ||
| <McpDeploymentsTable | ||
| deployments={[]} | ||
| toolbarContent={<div>toolbar</div>} | ||
| onClearFilters={onClearFilters} | ||
| onDeleteClick={onDeleteClick} | ||
| />, | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| expect(screen.queryByTestId('mcp-deployment-row-kubernetes-mcp')).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
This never asserts the empty-state view is actually present — it only checks a specific row is absent, which is trivially true when deployments={[]}. Consider asserting that DashboardEmptyTableView renders.
| it('should render column headers', () => { | ||
| render( | ||
| <McpDeploymentsTable | ||
| deployments={mockDeployments} | ||
| toolbarContent={<div>toolbar</div>} | ||
| onClearFilters={onClearFilters} | ||
| onDeleteClick={onDeleteClick} | ||
| />, | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| expect(screen.getByText('Server')).toBeInTheDocument(); | ||
| expect(screen.getByText('Name')).toBeInTheDocument(); | ||
| expect(screen.getByText('Created')).toBeInTheDocument(); | ||
| expect(screen.getByText('Status')).toBeInTheDocument(); | ||
| expect(screen.getByText('Service')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Low-value — tests that PatternFly renders column labels from a static config array. There's no branching logic under test; the labels come verbatim from the mcpDeploymentColumns constant.
| it('should render formatted creation date', () => { | ||
| renderRow(createMockDeployment()); | ||
| expect(screen.getByTestId('mcp-deployment-created')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Vacuous assertion — toBeInTheDocument() only checks the <Td> cell exists, not that the date is formatted correctly. Every other row test uses toHaveTextContent(...). Consider asserting the rendered date value.
| it('should render available status for Running phase', () => { | ||
| renderRow(createMockDeployment({ phase: McpDeploymentPhase.RUNNING })); | ||
| expect(screen.getByTestId('mcp-deployment-status-label')).toHaveTextContent('Available'); | ||
| }); | ||
|
|
||
| it('should render unavailable status for Failed phase', () => { | ||
| renderRow(createMockDeployment({ phase: McpDeploymentPhase.FAILED })); | ||
| expect(screen.getByTestId('mcp-deployment-status-label')).toHaveTextContent('Unavailable'); | ||
| }); | ||
|
|
||
| it('should render pending status for Pending phase', () => { | ||
| renderRow(createMockDeployment({ phase: McpDeploymentPhase.PENDING })); | ||
| expect(screen.getByTestId('mcp-deployment-status-label')).toHaveTextContent('Pending'); | ||
| }); |
There was a problem hiding this comment.
These three status-label tests duplicate the getStatusInfo coverage in utils.spec.ts, which already verifies the label text for each phase. These only add that McpDeploymentStatusLabel passes the value through to a <Label> — a single test would suffice for that wiring check.
| it('should have a row with the correct test id', () => { | ||
| renderRow(createMockDeployment()); | ||
| expect(screen.getByTestId('mcp-deployment-row-kubernetes-mcp')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Redundant — every other test in this file calls renderRow(createMockDeployment()) and queries elements inside the row; if the row didn't render they'd all fail. Also overlaps with "should render all deployment rows" in the table spec.
| it('should render View link for Running deployment', () => { | ||
| renderRow(createMockDeployment({ phase: McpDeploymentPhase.RUNNING })); | ||
| expect(screen.getByTestId('mcp-deployment-service-view')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should render View link with address URL when provided', () => { | ||
| renderRow( | ||
| createMockDeployment({ | ||
| phase: McpDeploymentPhase.RUNNING, | ||
| address: { url: 'kubernetes-test:8080' }, | ||
| }), | ||
| ); | ||
| expect(screen.getByTestId('mcp-deployment-service-view')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
These two tests have identical assertions (getByTestId('mcp-deployment-service-view').toBeInTheDocument()). The second test provides an address URL but never verifies the URL value — consider opening the popover and asserting the ClipboardCopy content to make the second test meaningful.
| it('should render dash for Failed deployment without address', () => { | ||
| renderRow(createMockDeployment({ phase: McpDeploymentPhase.FAILED })); | ||
| expect(screen.getByTestId('mcp-deployment-service-unavailable')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should render dash for Pending deployment without address', () => { | ||
| renderRow(createMockDeployment({ phase: McpDeploymentPhase.PENDING })); | ||
| expect(screen.getByTestId('mcp-deployment-service-unavailable')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Same logic path: getConnectionUrl returns undefined for any non-Running phase without an address → renders dash. Consider collapsing into it.each([FAILED, PENDING]). This is also already covered by getConnectionUrl tests in utils.spec.ts.
| it('should return address URL when provided', () => { | ||
| const deployment = createMockDeployment({ | ||
| phase: McpDeploymentPhase.RUNNING, | ||
| address: { url: 'https://kubernetes-mcp.example.com:8080' }, | ||
| }); | ||
| expect(getConnectionUrl(deployment)).toBe('https://kubernetes-mcp.example.com:8080'); | ||
| }); | ||
|
|
||
| it('should prefer address URL over name:port fallback', () => { | ||
| const deployment = createMockDeployment({ | ||
| phase: McpDeploymentPhase.RUNNING, | ||
| address: { url: 'custom-url:9090' }, | ||
| }); | ||
| expect(getConnectionUrl(deployment)).toBe('custom-url:9090'); |
There was a problem hiding this comment.
These two tests hit the same code branch (deployment.address?.url is truthy → return early). The name "should prefer address URL over name:port fallback" implies a precedence test, but there's no competing branch — the if short-circuits. One of these is sufficient.
| const result = getStatusInfo(McpDeploymentPhase.RUNNING); | ||
| expect(result.label).toBe('Available'); | ||
| expect(result.status).toBe('success'); | ||
| expect(result.tooltip).toBeTruthy(); |
There was a problem hiding this comment.
Weak assertion (repeated on lines 103, 110, 117) — toBeTruthy() passes for any non-empty string. Since getStatusInfo is a pure mapping with known outputs, assert the actual tooltip string.
manaswinidas
left a comment
manaswinidas
left a comment
There was a problem hiding this comment.
Flagging missing Cypress mock test coverage.
| ); | ||
| }; | ||
|
|
||
| export default McpDeploymentsPage; |
There was a problem hiding this comment.
Missing Cypress mock tests — this can be a follow-up, but flagging for visibility.
The MCP Catalog (sibling feature under the same mcp-servers nav section) has a full Cypress suite with page object, test utils, and intercepts (mcpCatalog.cy.ts, mcpCatalogTestUtils.ts, mcpCatalog.ts). The Deployments page has comparable interactive surface area but zero Cypress coverage.
Suggested coverage for a follow-up:
- Navigation & routing — nav item gated by
mcpCatalogflag, route renders page, unknown sub-routes redirect - Project selector — shows namespaces, switching re-fetches, disabled when
mandatoryNamespaceis set - Table rendering — rows from BFF response with correct server name, status labels, dates
- Service popover — "View" link opens popover with connection URL for Running; dash for Failed/Pending
- Filtering — filter by name/server name, clear filters restores list, no-match shows
DashboardEmptyTableView - Empty state — zero deployments shows
McpDeploymentsEmptyState - Error & loading states — BFF 500 shows error, pending request shows loading
- Kebab actions — Edit disabled with tooltip, Delete clickable
- Accessibility —
cy.testA11y()on main view and empty state
Test artifacts needed:
- Page object (
mcpDeployments.ts) withvisit(),wait()+cy.testA11y(), finder methods - Test utils (
mcpDeploymentsTestUtils.ts) withinitMcpDeploymentsIntercepts() - Test file (
mcpDeployments.cy.ts)
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6907 +/- ##
==========================================
+ Coverage 64.09% 64.46% +0.36%
==========================================
Files 2530 2497 -33
Lines 76695 76992 +297
Branches 19202 19127 -75
==========================================
+ Hits 49161 49635 +474
+ Misses 27534 27357 -177
... and 118 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
2 participants
https://redhat.atlassian.net/browse/RHOAIENG-53379
Description
Adds the MCP Deployments list page — the primary view for managing deployed MCP servers. This includes navigation registration, the page shell, the deployments table, and all supporting components.
How Has This Been Tested?
mcpCatalogfeature flag is enablednpx jest --testPathPattern=mcpDeployments)curlTest Impact
Added 3 new test files with 29 tests:
__tests__/utils.spec.ts— Tests forgetServerDisplayName,getStatusInfo, andgetConnectionUrlutility functions (edge cases, all phase mappings)__tests__/McpDeploymentsTableRow.spec.tsx— Tests for row rendering (server name, deployment name, date, status labels for all phases, Service View/dash for available/unavailable)__tests__/McpDeploymentsTable.spec.tsx— Tests for table rendering (all rows present, column headers, empty state)Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main