feat(UI): Add gateways table#4537
Conversation
08b7b7b to
dc986f0
Compare
dc986f0 to
deb55e4
Compare
93c1415 to
e47f730
Compare
vishu-bh
left a comment
There was a problem hiding this comment.
Nice progress on the gateways table. I found a couple of blocking runtime issues worth tightening before merge:
- client/src/pages/Servers.tsx - The table pagination looks like it does not match the current /gateways API contract. The UI sends page / per_page and then derives total_pages: 1, while the backend route appears to use cursor/limit pagination. As a result, users may only ever see the first backend page and won’t be able to reach later gateways. It may be worth switching the UI to use limit + nextCursor, or explicitly adding page pagination support to the API route.
This one is worth checking -
- client/src/pages/Servers.tsx - The “Test Connection” action calls POST /gateways/{id}/test, but I couldn’t find a matching backend route for that endpoint. The existing gateway test endpoint seems to be POST /admin/gateways/test, with a different request shape. As-is, this action looks like it will fail at runtime for every gateway.
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
e47f730 to
495cb1d
Compare
c6bc408 to
495cb1d
Compare
| "@testing-library/react": "^16.1.0", | ||
| "@testing-library/user-event": "^14.5.2", | ||
| "@types/node": "^25.6.0", | ||
| "@types/prop-types": "^15.7.15", |
There was a problem hiding this comment.
Do we need to have prop-types dep? The Prop types should be handled by TS.
There was a problem hiding this comment.
Unforfunately, TS was complaining about the className type not being defined. I accepted the tradeoff as a dev dependancy.
| "requires": true, | ||
| "packages": { | ||
| "": { | ||
| "name": "mcp-context-forge", |
There was a problem hiding this comment.
I have no idea, tbh. It was removed automatically, I'll put it back. 👍
| }, [state.user]); | ||
|
|
||
| const login = useCallback(async (email: string, password: string): Promise<void> => { | ||
| /* prettier-ignore */ const login = useCallback(async (email: string, password: string): Promise<void> => { // pragma: allowlist secret |
There was a problem hiding this comment.
Do we need to have /* prettier-ignore */ here?
There was a problem hiding this comment.
Yes, for the "pragma: allowlist secret", otherwise prettier pushes that to the next line and we have to add that to .secres.baseline. :(
There was a problem hiding this comment.
Actually, found a way to get it through without the ignore:
const login = useCallback(
async (
email: string,
password: string, // pragma: allowlist secret
): Promise<void> => {
const data = await api.post<LoginResponse>(
"/auth/login",
{ email, password },
{ unauthenticated: true },
);
setToken(data.access_token);
setState({ user: data.user, isAuthenticated: true });
},
[],
);| TableHead.displayName = "TableHead"; | ||
| TableHead.propTypes = { | ||
| className: PropTypes.string, | ||
| }; |
There was a problem hiding this comment.
I think all displayName and propTypes should be handled by TS instead of PropTypes dep.
There was a problem hiding this comment.
I'll try to resolve without PropTypes, then get back here with the results.
| vi.mock("@/api/client", () => ({ | ||
| api: { | ||
| get: vi.fn(), | ||
| delete: vi.fn(), | ||
| post: vi.fn(), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
We can remove this mock with MSW one, where we can define the responses.
We can mock the response in the file /client/src/test/mocks/handlers.ts
There was a problem hiding this comment.
TL;DR: MSW (Mock Service Worker) has compatibility issues with AbortSignal in Node.js test environments, causing tests to fail. We've switched to direct API client mocking as a workaround.
Technical Details
The Problem:
- Our API client (@/api/client) uses AbortSignal for request cancellation (standard fetch API feature)
- MSW intercepts HTTP requests at the network level in Node.js using the http/https modules
- Node.js's native HTTP modules don't fully support AbortSignal in the same way browsers do
- When MSW tries to intercept requests with AbortSignal, it causes compatibility errors in the test environment
The Solution:
Instead of using MSW to mock HTTP responses, we:
- Mock the @/api/client module directly using Vitest's vi.mock()
- Control the mock responses at the API client level, before any HTTP layer is involved
- This bypasses the AbortSignal incompatibility entirely
Code Example:
1 // Mock the entire api client module
2 vi.mock("@/api/client", () => ({
3 api: {
4 get: vi.fn(),
5 delete: vi.fn(),
6 post: vi.fn(),
7 },
8 }));
9
10 // Then control responses in tests
11 vi.mocked(api.get).mockResolvedValue({ data: [...] });
Trade-offs:
- ✅ Tests run reliably without AbortSignal issues
- ✅ Faster test execution (no network layer simulation)
- ✅ Simpler test setup
⚠️ We're not testing the actual HTTP client behavior (but that's covered by integration tests)
This is a pragmatic solution that keeps unit tests focused on component logic while avoiding Node.js/MSW compatibility issues.
There was a problem hiding this comment.
This is what bob says.
| vi.mock("@/router", () => ({ | ||
| useRouter: () => ({ | ||
| navigate: mockNavigate, | ||
| path: "/app/servers", | ||
| params: {}, | ||
| }), | ||
| })); |
There was a problem hiding this comment.
Is it possible to use the real router instead of mocking it?
| page: currentPage, | ||
| per_page: perPage, | ||
| total: response.gateways.length, | ||
| total_pages: 1, |
There was a problem hiding this comment.
Can we replace this hardcoded value with the value from API?
| ? { | ||
| page: currentPage, | ||
| per_page: perPage, | ||
| total: response.gateways.length, |
There was a problem hiding this comment.
Are we not getting the per_page value from API?
|
|
||
| export interface ServersResponse { | ||
| gateways: MCPServer[]; | ||
| nextCursor?: string | null; |
There was a problem hiding this comment.
Is it the correct type string? I'd expect number instead.
There was a problem hiding this comment.
From our openapi.json:
{
"CursorPaginatedGatewaysResponse": {
"properties": {
"gateways": {
"items": {
"$ref": "#/components/schemas/GatewayRead"
},
"type": "array",
"title": "Gateways",
"description": "List of gateways for this page"
},
"nextCursor": {
"anyOf": [
{
"type": "string"
},
{
"type": "null"
}
],
"title": "Nextcursor",
"description": "Cursor for the next page, null if no more pages"
}
},
"type": "object",
"required": [
"gateways"
],
"title": "CursorPaginatedGatewaysResponse",
"description": "Cursor-paginated response for gateways list endpoint."
}
}0ca827d to
e888144
Compare
e888144 to
b816e92
Compare
| "@testing-library/react": "^16.1.0", | ||
| "@testing-library/user-event": "^14.5.2", | ||
| "@types/node": "^25.6.0", | ||
| "@types/prop-types": "^15.7.15", |
There was a problem hiding this comment.
prop-types dep is still here. I think you need to run npm install to remove it
Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
b816e92 to
349d5bb
Compare
marekdano
left a comment
There was a problem hiding this comment.
It looks good!
LGTM 🚀
* feat(ui): Add gateways table Signed-off-by: Gabriel Costa <gabrielcg@proton.me> * fix(ui): change pagination to cursor pattern Signed-off-by: Gabriel Costa <gabrielcg@proton.me> --------- Signed-off-by: Gabriel Costa <gabrielcg@proton.me>
closes #4172