[UI-REWRITE]: Improve unit test coverage for React UI client to 95% and remove dead code (#5561)

* test: Add comprehensive coverage for hooks and gateways components

Signed-off-by: Prince Patel <princebpatel2005@gmail.com>

* test: fix Resource MSW mocks and Server tests, clean dead code

Signed-off-by: Prince Patel <princebpatel2005@gmail.com>

* chore: remove singleFork and unused code

Signed-off-by: Prince Patel <princebpatel2005@gmail.com>

* style: auto-fix linting issues

Signed-off-by: Prince Patel <princebpatel2005@gmail.com>

* fix: resolve TypeScript errors from upstream type changes

Signed-off-by: Prince Patel <princebpatel2005@gmail.com>

* chore(ui-rewrite): clean up test-coverage PR per review and rebase onto epic/ui-rewrite

Signed-off-by: Marek Dano <Marek.Dano@ibm.com>

* test(ui-rewrite): consolidate duplicate test files and fix tsc build errors

Signed-off-by: Marek Dano <Marek.Dano@ibm.com>

* test(ui-rewrite): raise UI test coverage and enforce thresholds in CI

Signed-off-by: Marek Dano <Marek.Dano@ibm.com>

---------

Signed-off-by: Prince Patel <princebpatel2005@gmail.com>
Signed-off-by: Marek Dano <Marek.Dano@ibm.com>
Co-authored-by: Marek Dano <Marek.Dano@ibm.com>

Author: 2 people

.github/workflows/client-lint-test.yml

@@ -118,4 +118,4 @@ jobs:
       # -----------------------------------------------------------
       - name: 🧪  Run tests with Vitest
         working-directory: ./client
-        run: npm run test:run
+        run: npm run test:coverage

.gitignore

@@ -120,6 +120,7 @@ playwright-report-*.html
 # Vitest
 coverage/
 .vitest/
+client/vitest.config.ts.timestamp-*.mjs
 
 # ========================================
 # Fuzzing

client/src/App.test.tsx

@@ -1,5 +1,5 @@
 import { describe, it, expect, vi } from "vitest";
-import { screen } from "@testing-library/react";
+import { screen, waitFor } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 import { App } from "./App";
 import { renderWithProviders } from "./test/test-utils";
@@ -51,4 +51,15 @@ describe("App", () => {
     });
     expect(gatewaysHeading).toBeInTheDocument();
   });
+
+  it("redirects bare /app to /app/", async () => {
+    localStorage.clear();
+    sessionStorage.clear();
+    window.history.pushState({}, "", "/app");
+
+    renderWithProviders(<App />);
+
+    // The bare /app path renders <Redirect to="/app/" />, which normalizes the URL.
+    await waitFor(() => expect(window.location.pathname).toBe("/app/"));
+  });
 });

client/src/api/resources.test.ts

@@ -56,4 +56,73 @@ describe("resourcesApi", () => {
       await expect(resourcesApi.updateTags("42", ["x"])).rejects.toThrow("HTTP 403");
     });
   });
+
+  const okJson = (body: unknown) =>
+    new Response(JSON.stringify(body), {
+      status: 200,
+      headers: { "Content-Type": "application/json" },
+    });
+
+  describe("create", () => {
+    it("POSTs the resource to /resources", async () => {
+      mockFetch.mockResolvedValueOnce(okJson({ id: "new-resource" }));
+
+      await resourcesApi.create({
+        uri: "resource://example",
+        name: "Example",
+        content: "hello",
+      } as Parameters<typeof resourcesApi.create>[0]);
+
+      expect(mockFetch).toHaveBeenCalledWith(
+        expect.stringContaining("/resources"),
+        expect.objectContaining({ method: "POST" }),
+      );
+    });
+  });
+
+  describe("update", () => {
+    it("PUTs the resource to /resources/:id", async () => {
+      mockFetch.mockResolvedValueOnce(okJson({ id: "res-1" }));
+
+      await resourcesApi.update("res-1", { name: "Renamed" } as Parameters<
+        typeof resourcesApi.update
+      >[1]);
+
+      expect(mockFetch).toHaveBeenCalledWith(
+        expect.stringContaining("/resources/res-1"),
+        expect.objectContaining({ method: "PUT" }),
+      );
+    });
+
+    it("throws synchronously for an invalid ID", () => {
+      expect(() => resourcesApi.update("../etc/passwd", {})).toThrow("Invalid resource ID format");
+    });
+  });
+
+  describe("delete", () => {
+    it("DELETEs /resources/:id", async () => {
+      mockFetch.mockResolvedValueOnce(okJson({}));
+
+      await resourcesApi.delete("res-1");
+
+      expect(mockFetch).toHaveBeenCalledWith(
+        expect.stringContaining("/resources/res-1"),
+        expect.objectContaining({ method: "DELETE" }),
+      );
+    });
+
+    it("throws synchronously for an invalid ID", () => {
+      expect(() => resourcesApi.delete("bad/id")).toThrow("Invalid resource ID format");
+    });
+  });
+
+  describe("validateResourceId (via delete)", () => {
+    it("rejects an empty id", () => {
+      expect(() => resourcesApi.delete("")).toThrow(/^Invalid resource ID$/);
+    });
+
+    it("rejects an id longer than 255 characters", () => {
+      expect(() => resourcesApi.delete("a".repeat(256))).toThrow("Resource ID too long");
+    });
+  });
 });

client/src/api/servers.test.ts

@@ -1,5 +1,6 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
 import { serversApi } from "./servers";
+import type { GatewayTestRequest } from "@/generated/types";
 
 describe("serversApi", () => {
   const mockFetch = vi.fn();
@@ -255,6 +256,31 @@ describe("serversApi", () => {
       vi.useRealTimers();
     });
 
+    it("ignores non-oauth_callback messages from the popup", async () => {
+      vi.useFakeTimers();
+      const mockAuthWindow = { closed: false } as unknown as Window;
+      vi.spyOn(window, "open").mockReturnValue(mockAuthWindow);
+
+      const promise = serversApi.triggerOAuthAuthorization("server-123");
+
+      // Correct source, but payloads that fail the null / type guard are ignored.
+      const nullEvent = new MessageEvent("message", { data: null });
+      Object.defineProperty(nullEvent, "source", { value: mockAuthWindow, writable: false });
+      window.dispatchEvent(nullEvent);
+
+      const wrongTypeEvent = new MessageEvent("message", { data: { type: "something_else" } });
+      Object.defineProperty(wrongTypeEvent, "source", { value: mockAuthWindow, writable: false });
+      window.dispatchEvent(wrongTypeEvent);
+
+      // Neither settled the promise; closing the popup does.
+      (mockAuthWindow as { closed: boolean }).closed = true;
+      vi.advanceTimersByTime(1000);
+
+      await expect(promise).rejects.toThrow("OAuth authorization was cancelled");
+
+      vi.useRealTimers();
+    });
+
     it("rejects with cancellation message when user closes the popup", async () => {
       vi.useFakeTimers();
       const mockAuthWindow = { closed: false } as unknown as Window;
@@ -299,4 +325,193 @@ describe("serversApi", () => {
       );
     });
   });
+
+  describe("validateServerId (via get)", () => {
+    it("throws 'Invalid server ID' for an empty id", () => {
+      expect(() => serversApi.get("")).toThrow(/^Invalid server ID$/);
+    });
+
+    it("throws 'Invalid server ID format' for an id with illegal characters", () => {
+      expect(() => serversApi.get("bad/id")).toThrow("Invalid server ID format");
+    });
+  });
+
+  describe("list", () => {
+    const jsonResponse = (body: unknown) =>
+      new Response(JSON.stringify(body), {
+        status: 200,
+        headers: { "Content-Type": "application/json" },
+      });
+
+    it("requests /gateways with only include_pagination by default", async () => {
+      const body = { servers: [], pagination: { nextCursor: null } };
+      mockFetch.mockResolvedValueOnce(jsonResponse(body));
+
+      const result = await serversApi.list();
+
+      expect(result).toEqual(body);
+      const url = mockFetch.mock.calls[0][0] as string;
+      expect(url).toContain("/gateways?");
+      expect(url).toContain("include_pagination=true");
+      expect(url).not.toContain("cursor=");
+      expect(url).not.toContain("limit=");
+      expect(url).not.toContain("include_inactive=");
+    });
+
+    it("includes cursor, clamped limit, and include_inactive when provided", async () => {
+      mockFetch.mockResolvedValueOnce(jsonResponse({ servers: [] }));
+
+      await serversApi.list({ cursor: "next-page", limit: 500, include_inactive: true });
+
+      const url = mockFetch.mock.calls[0][0] as string;
+      expect(url).toContain("cursor=next-page");
+      expect(url).toContain("limit=100"); // clamped to the max of 100
+      expect(url).toContain("include_inactive=true");
+    });
+
+    it("clamps a limit below 1 up to 1", async () => {
+      mockFetch.mockResolvedValueOnce(jsonResponse({ servers: [] }));
+
+      await serversApi.list({ limit: 0 });
+
+      expect(mockFetch.mock.calls[0][0]).toContain("limit=1");
+    });
+
+    it("falls back to a limit of 25 for a non-finite limit", async () => {
+      mockFetch.mockResolvedValueOnce(jsonResponse({ servers: [] }));
+
+      await serversApi.list({ limit: Number.POSITIVE_INFINITY });
+
+      expect(mockFetch.mock.calls[0][0]).toContain("limit=25");
+    });
+
+    it("resolves when passed an AbortSignal", async () => {
+      mockFetch.mockResolvedValueOnce(jsonResponse({ servers: [] }));
+      const controller = new AbortController();
+
+      await expect(serversApi.list({ signal: controller.signal })).resolves.toEqual({
+        servers: [],
+      });
+    });
+  });
+
+  describe("get", () => {
+    const jsonResponse = (body: unknown) =>
+      new Response(JSON.stringify(body), {
+        status: 200,
+        headers: { "Content-Type": "application/json" },
+      });
+
+    it("fetches /gateways/:id and returns the server", async () => {
+      const server = { id: "get-basic", name: "Basic" };
+      mockFetch.mockResolvedValueOnce(jsonResponse(server));
+
+      const result = await serversApi.get("get-basic");
+
+      expect(result).toEqual(server);
+      expect(mockFetch).toHaveBeenCalledWith(
+        expect.stringContaining("/gateways/get-basic"),
+        expect.objectContaining({ method: "GET" }),
+      );
+    });
+
+    it("returns the same in-flight promise for concurrent gets (request cache)", async () => {
+      mockFetch.mockResolvedValueOnce(jsonResponse({ id: "get-cache" }));
+
+      const first = serversApi.get("get-cache");
+      const second = serversApi.get("get-cache");
+
+      expect(first).toBe(second);
+      await first;
+      expect(mockFetch).toHaveBeenCalledTimes(1);
+    });
+
+    it("evicts the cache on failure so a later call retries", async () => {
+      mockFetch.mockResolvedValueOnce(
+        new Response(JSON.stringify({ detail: "boom" }), {
+          status: 500,
+          headers: { "Content-Type": "application/json" },
+        }),
+      );
+
+      await expect(serversApi.get("get-evict")).rejects.toThrow("HTTP 500");
+
+      // The failed request was removed from the cache, so this refetches.
+      mockFetch.mockResolvedValueOnce(jsonResponse({ id: "get-evict" }));
+      const result = await serversApi.get("get-evict");
+
+      expect(result).toEqual({ id: "get-evict" });
+      expect(mockFetch).toHaveBeenCalledTimes(2);
+    });
+  });
+
+  describe("testConnection", () => {
+    it("POSTs /gateways/:id/test and returns the result", async () => {
+      mockFetch.mockResolvedValueOnce(
+        new Response(JSON.stringify({ success: true, message: "Reachable" }), {
+          status: 200,
+          headers: { "Content-Type": "application/json" },
+        }),
+      );
+
+      const result = await serversApi.testConnection("server-123");
+
+      expect(result).toEqual({ success: true, message: "Reachable" });
+      expect(mockFetch).toHaveBeenCalledWith(
+        expect.stringContaining("/gateways/server-123/test"),
+        expect.objectContaining({ method: "POST" }),
+      );
+    });
+
+    it("throws synchronously for an invalid server ID", () => {
+      expect(() => serversApi.testConnection("../etc/passwd")).toThrow("Invalid server ID format");
+    });
+  });
+
+  describe("delete", () => {
+    it("DELETEs /gateways/:id", async () => {
+      mockFetch.mockResolvedValueOnce(
+        new Response(JSON.stringify({}), {
+          status: 200,
+          headers: { "Content-Type": "application/json" },
+        }),
+      );
+
+      await serversApi.delete("server-123");
+
+      expect(mockFetch).toHaveBeenCalledWith(
+        expect.stringContaining("/gateways/server-123"),
+        expect.objectContaining({ method: "DELETE" }),
+      );
+    });
+
+    it("throws synchronously for an invalid server ID", () => {
+      expect(() => serversApi.delete("../etc/passwd")).toThrow("Invalid server ID format");
+    });
+  });
+
+  describe("testConnectivity", () => {
+    it("POSTs the request to /v1/mcp-servers/test and returns the response", async () => {
+      const upstream = { statusCode: 200, body: "ok" };
+      mockFetch.mockResolvedValueOnce(
+        new Response(JSON.stringify(upstream), {
+          status: 200,
+          headers: { "Content-Type": "application/json" },
+        }),
+      );
+
+      const request: GatewayTestRequest = {
+        method: "GET",
+        baseUrl: "https://example.com",
+        path: "/health",
+      };
+      const result = await serversApi.testConnectivity(request);
+
+      expect(result).toEqual(upstream);
+      expect(mockFetch).toHaveBeenCalledWith(
+        expect.stringContaining("/v1/mcp-servers/test"),
+        expect.objectContaining({ method: "POST", body: JSON.stringify(request) }),
+      );
+    });
+  });
 });