fix(FN-1895): improve companies.sh import error handling and add retry behavior

- Surface companies.sh API errors to frontend instead of returning empty list - Add 30s timeout to GitHub archive download to prevent hanging - Prevent infinite retry loop in browse catalog useEffect - Update retry handler to call fetch directly for better control - Fix TypeScript error with Response type in timeout code - Add comprehensive tests for error handling and retry behavior
2026-04-16 13:31:47 -07:00
parent 222a807dc2
commit 0bbdb87e55
6 changed files with 307 additions and 17 deletions
--- a/packages/dashboard/app/api.ts
+++ b/packages/dashboard/app/api.ts
@@ -2707,6 +2707,12 @@ export interface CompanyEntry {
  installs?: number;
 }

+/** Response from companies.sh catalog API */
+export interface CompaniesCatalogResponse {
+  companies: CompanyEntry[];
+  error?: string;
+}
+
 /** Result of importing agents from an Agent Companies source */
 export interface AgentImportResult {
  companyName?: string;
@@ -2721,9 +2727,10 @@ export interface AgentImportResult {

 /**
 * Fetch companies from companies.sh catalog.
+ * Returns both companies and optional error message for proper error surfacing.
 */
-export function fetchCompanies(): Promise<CompanyEntry[]> {
-  return api<{ companies: CompanyEntry[] }>("/agents/companies").then((res) => res.companies);
+export function fetchCompanies(): Promise<CompaniesCatalogResponse> {
+  return api<CompaniesCatalogResponse>("/agents/companies");
 }

 /**
--- a/packages/dashboard/app/components/AgentImportModal.tsx
+++ b/packages/dashboard/app/components/AgentImportModal.tsx
@@ -1,5 +1,5 @@
 import { useState, useRef, useCallback, useEffect } from "react";
-import { Upload, FileText, CheckCircle, AlertTriangle, X, Loader2, FolderOpen, Globe, Search } from "lucide-react";
+import { Upload, FileText, CheckCircle, AlertTriangle, X, Loader2, FolderOpen, Globe, Search, RefreshCw } from "lucide-react";
 import { fetchCompanies, type CompanyEntry } from "../api";

 export interface AgentImportModalProps {
@@ -133,14 +133,24 @@ export function AgentImportModal({ isOpen, onClose, onImported, projectId }: Age
  const [isLoadingCompanies, setIsLoadingCompanies] = useState(false);
  const [companiesError, setCompaniesError] = useState<string | null>(null);

+  // Track whether we've attempted to fetch to prevent infinite retry loops
+  const fetchAttemptedRef = useRef(false);
+
  // Load companies when browse mode is selected
  useEffect(() => {
-    if (inputMethod === "browse" && companies.length === 0 && !isLoadingCompanies) {
+    if (inputMethod === "browse" && !fetchAttemptedRef.current && !isLoadingCompanies) {
+      fetchAttemptedRef.current = true;
      setIsLoadingCompanies(true);
      setCompaniesError(null);
      fetchCompanies()
        .then((data) => {
-          setCompanies(data);
+          if (data.error) {
+            setCompaniesError(data.error);
+          } else if (data.companies.length > 0) {
+            setCompanies(data.companies);
+          } else {
+            setCompaniesError("No companies available");
+          }
        })
        .catch((err) => {
          setCompaniesError(err instanceof Error ? err.message : "Failed to load companies");
@@ -149,7 +159,32 @@ export function AgentImportModal({ isOpen, onClose, onImported, projectId }: Age
          setIsLoadingCompanies(false);
        });
    }
-  }, [inputMethod, companies.length, isLoadingCompanies]);
+  }, [inputMethod, isLoadingCompanies]);
+
+  /** Retry fetching companies after an error - calls fetch directly to bypass useEffect */
+  const handleRetryFetchCompanies = useCallback(() => {
+    fetchAttemptedRef.current = true; // Prevent useEffect from also firing
+    setCompaniesError(null);
+    setCompanies([]);
+    setSelectedCompany(null);
+    setIsLoadingCompanies(true);
+    fetchCompanies()
+      .then((data) => {
+        if (data.error) {
+          setCompaniesError(data.error);
+        } else if (data.companies.length > 0) {
+          setCompanies(data.companies);
+        } else {
+          setCompaniesError("No companies available");
+        }
+      })
+      .catch((err) => {
+        setCompaniesError(err instanceof Error ? err.message : "Failed to load companies");
+      })
+      .finally(() => {
+        setIsLoadingCompanies(false);
+      });
+  }, []);

  const reset = useCallback(() => {
    setStep("input");
@@ -168,6 +203,7 @@ export function AgentImportModal({ isOpen, onClose, onImported, projectId }: Age
    setSelectedCompany(null);
    setIsLoadingCompanies(false);
    setCompaniesError(null);
+    fetchAttemptedRef.current = false;
  }, []);

  const handleClose = useCallback(() => {
@@ -454,6 +490,14 @@ export function AgentImportModal({ isOpen, onClose, onImported, projectId }: Age
                    <div className="agent-import-browse-error">
                      <AlertTriangle size={16} />
                      <span>{companiesError}</span>
+                      <button
+                        type="button"
+                        className="btn btn-sm"
+                        onClick={handleRetryFetchCompanies}
+                      >
+                        <RefreshCw size={14} />
+                        Retry
+                      </button>
                    </div>
                  )}

--- a/packages/dashboard/app/components/tests/AgentImportModal.test.tsx
+++ b/packages/dashboard/app/components/tests/AgentImportModal.test.tsx
@@ -1,5 +1,5 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
-import { render, screen, waitFor, fireEvent } from "@testing-library/react";
+import { render, screen, waitFor, fireEvent, act } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 import { AgentImportModal } from "../AgentImportModal";

@@ -285,4 +285,147 @@ describe("AgentImportModal", () => {
    fireEvent.click(overlay!);
    expect(onClose).toHaveBeenCalledTimes(1);
  });
+
+  describe("Browse Catalog Mode", () => {
+    function createMockResponse(body: unknown): Response {
+      return {
+        ok: true,
+        status: 200,
+        headers: new globalThis.Headers({ "content-type": "application/json" }),
+        text: async () => JSON.stringify(body),
+        json: async () => body,
+      } as unknown as Response;
+    }
+
+    it("shows companies when fetch succeeds", async () => {
+      const mockCompanies = [
+        { slug: "test-company", name: "Test Company", tagline: "A great company" },
+        { slug: "another-co", name: "Another Company" },
+      ];
+
+      globalThis.fetch = vi.fn().mockResolvedValue(createMockResponse({ companies: mockCompanies }));
+
+      renderModal(true);
+
+      const user = userEvent.setup();
+      await user.click(screen.getByRole("button", { name: "Browse Catalog" }));
+
+      await waitFor(() => {
+        expect(screen.getByText("Test Company")).toBeInTheDocument();
+        expect(screen.getByText("Another Company")).toBeInTheDocument();
+      });
+    });
+
+    it("shows error message when companies.sh returns error", async () => {
+      globalThis.fetch = vi.fn().mockResolvedValue(createMockResponse({
+        companies: [],
+        error: "Failed to fetch companies.sh catalog: Network unreachable",
+      }));
+
+      renderModal(true);
+
+      const user = userEvent.setup();
+      await user.click(screen.getByRole("button", { name: "Browse Catalog" }));
+
+      await waitFor(() => {
+        expect(screen.getByText(/Failed to fetch companies.sh catalog/)).toBeInTheDocument();
+      });
+    });
+
+    it("shows Retry button when error occurs", async () => {
+      globalThis.fetch = vi.fn().mockResolvedValue(createMockResponse({
+        companies: [],
+        error: "Failed to fetch companies.sh catalog: Network unreachable",
+      }));
+
+      renderModal(true);
+
+      const user = userEvent.setup();
+      await user.click(screen.getByRole("button", { name: "Browse Catalog" }));
+
+      await waitFor(() => {
+        expect(screen.getByRole("button", { name: /Retry/i })).toBeInTheDocument();
+      });
+    });
+
+    it("does not retry infinitely after error", async () => {
+      let fetchCallCount = 0;
+      globalThis.fetch = vi.fn().mockImplementation(() => {
+        fetchCallCount++;
+        return Promise.resolve(createMockResponse({
+          companies: [],
+          error: "Network unreachable",
+        }));
+      });
+
+      renderModal(true);
+
+      const user = userEvent.setup();
+      await user.click(screen.getByRole("button", { name: "Browse Catalog" }));
+
+      // Wait for the error to appear
+      await waitFor(() => {
+        expect(screen.getByText(/Network unreachable/)).toBeInTheDocument();
+      });
+
+      // Allow a bit of time for any potential extra fetches
+      await new Promise((resolve) => setTimeout(resolve, 500));
+
+      // Should have exactly 1 fetch call (no infinite retry)
+      expect(fetchCallCount).toBe(1);
+    });
+
+    it("retry button allows re-fetching after error", async () => {
+      // Use a ref to track call count
+      const callCountRef = { current: 0 };
+
+      globalThis.fetch = vi.fn().mockImplementation(() => {
+        callCountRef.current++;
+        const callNum = callCountRef.current;
+
+        // First call returns error, subsequent calls return success
+        if (callNum === 1) {
+          return Promise.resolve(createMockResponse({
+            companies: [],
+            error: "Network unreachable",
+          }));
+        }
+        return Promise.resolve(createMockResponse({
+          companies: [{ slug: "test-co", name: "Test Company" }],
+        }));
+      });
+
+      renderModal(true);
+
+      const user = userEvent.setup();
+      await user.click(screen.getByRole("button", { name: "Browse Catalog" }));
+
+      // Wait for error
+      await waitFor(() => {
+        expect(screen.getByText(/Network unreachable/)).toBeInTheDocument();
+      });
+
+      // Verify only 1 fetch happened
+      expect(callCountRef.current).toBe(1);
+
+      // Click retry using act to ensure state updates are processed
+      const retryButton = screen.getByRole("button", { name: /Retry/i });
+      await act(async () => {
+        await user.click(retryButton);
+      });
+
+      // Wait for the error to be cleared (indicating retry is in progress)
+      await waitFor(() => {
+        expect(screen.queryByText(/Network unreachable/)).not.toBeInTheDocument();
+      });
+
+      // The mock should return success on second call, so we should see the companies
+      await waitFor(() => {
+        expect(screen.getByText("Test Company")).toBeInTheDocument();
+      });
+
+      // Verify a second fetch happened
+      expect(callCountRef.current).toBe(2);
+    });
+  });
 });
--- a/packages/dashboard/src/tests/routes-agent-import.test.ts
+++ b/packages/dashboard/src/tests/routes-agent-import.test.ts
@@ -374,6 +374,7 @@ describe("POST /api/agents/import", () => {
 describe("GET /api/agents/companies", () => {
  let store: MockStore;
  let app: ReturnType<typeof import("../server.js").createServer>;
+  const originalFetch = globalThis.fetch;

  beforeEach(async () => {
    vi.clearAllMocks();
@@ -391,13 +392,69 @@ describe("GET /api/agents/companies", () => {

  afterEach(() => {
    vi.restoreAllMocks();
+    globalThis.fetch = originalFetch;
  });

-  it("returns companies from companies.sh API", async () => {
+  it("returns companies when external API succeeds", async () => {
+    const mockCompanies = [
+      { slug: "test-company", name: "Test Company", tagline: "A test company" },
+      { slug: "another-company", name: "Another Company" },
+    ];
+
+    globalThis.fetch = vi.fn().mockResolvedValue({
+      ok: true,
+      status: 200,
+      headers: { get: () => "application/json" },
+      json: async () => mockCompanies,
+    });
+
    const response = await request(app, "GET", "/api/agents/companies");

-    // The actual API might return data or an empty array on failure
-    // Just verify the endpoint responds
-    expect([200, 500]).toContain(response.status);
+    expect(response.status).toBe(200);
+    const body = response.body as any;
+    expect(body.companies).toHaveLength(2);
+    expect(body.companies[0].slug).toBe("test-company");
+    expect(body.error).toBeUndefined();
+  });
+
+  it("returns error message when external API is unreachable", async () => {
+    globalThis.fetch = vi.fn().mockRejectedValue(new Error("Network unreachable"));
+
+    const response = await request(app, "GET", "/api/agents/companies");
+
+    expect(response.status).toBe(200);
+    const body = response.body as any;
+    expect(body.companies).toEqual([]);
+    expect(body.error).toContain("Failed to fetch companies.sh catalog:");
+    expect(body.error).toContain("Network unreachable");
+  });
+
+  it("returns error message when external API returns non-JSON", async () => {
+    globalThis.fetch = vi.fn().mockResolvedValue({
+      ok: true,
+      status: 200,
+      headers: { get: () => "text/html" },
+      json: async () => { throw new Error("Not JSON"); },
+    });
+
+    const response = await request(app, "GET", "/api/agents/companies");
+
+    expect(response.status).toBe(200);
+    const body = response.body as any;
+    expect(body.companies).toEqual([]);
+    expect(body.error).toContain("Failed to fetch companies.sh catalog:");
+  });
+
+  it("returns 500 when request times out", async () => {
+    // Create an AbortError by using a mock that throws with 'aborted' in the message
+    const abortError = new Error("The operation was aborted");
+    abortError.name = "AbortError";
+    globalThis.fetch = vi.fn().mockRejectedValue(abortError);
+
+    const response = await request(app, "GET", "/api/agents/companies");
+
+    expect(response.status).toBe(500);
+    const body = response.body as any;
+    expect(body.error).toContain("timed out");
  });
 });
--- a/packages/dashboard/src/routes.ts
+++ b/packages/dashboard/src/routes.ts
@@ -10638,13 +10638,15 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
          }).filter((c): c is CompaniesShCompany => c !== null);
        }
      } catch (fetchErr) {
-        // Return empty array on network/parsing errors (graceful degradation)
+        // Return empty array + error message on network/parsing errors
        const message = fetchErr instanceof Error ? fetchErr.message : String(fetchErr);
        if (message.includes("aborted")) {
          throw new Error("companies.sh request timed out");
        }
-        // Log but don't fail - return empty catalog
+        // Log and include error in response so frontend can display it
        console.warn(`[agents/companies] Failed to fetch catalog: ${message}`);
+        res.json({ companies, error: `Failed to fetch companies.sh catalog: ${message}` });
+        return;
      }

      res.json({ companies });
@@ -10824,10 +10826,34 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
          // Download the archive
          const archivePath = join(tempDir, "archive.tar.gz");

-          const archiveResponse = await fetch(archiveUrl);
-          const downloadResponse = archiveResponse.ok
-            ? archiveResponse
-            : await fetch(`https://github.com/${repoOwner}/${repoName}/archive/refs/heads/master.tar.gz`);
+          // Download with 30-second timeout
+          const downloadController = new AbortController();
+          const downloadTimeout = setTimeout(() => downloadController.abort(), 30000);
+
+          let archiveResponse: globalThis.Response;
+          try {
+            archiveResponse = await fetch(archiveUrl, { signal: downloadController.signal });
+          } finally {
+            clearTimeout(downloadTimeout);
+          }
+
+          let downloadResponse: globalThis.Response;
+          if (archiveResponse.ok) {
+            downloadResponse = archiveResponse;
+          } else {
+            // Try fallback branch (master) with its own timeout
+            const fallbackController = new AbortController();
+            const fallbackTimeout = setTimeout(() => fallbackController.abort(), 30000);
+
+            try {
+              downloadResponse = await fetch(
+                `https://github.com/${repoOwner}/${repoName}/archive/refs/heads/master.tar.gz`,
+                { signal: fallbackController.signal },
+              );
+            } finally {
+              clearTimeout(fallbackTimeout);
+            }
+          }

          if (!downloadResponse.ok) {
            throw badRequest(`Failed to download repository archive: ${downloadResponse.status} ${downloadResponse.statusText}`);
@@ -11010,6 +11036,10 @@ export function createApiRoutes(store: TaskStore, options?: ServerOptions): Rout
      if (err instanceof Error && err.name === "AgentCompaniesParseError") {
        throw badRequest(err instanceof Error ? err.message : String(err));
      }
+      // Handle AbortError from timed-out fetch calls
+      if (err instanceof Error && err.name === "AbortError") {
+        throw badRequest("Downloading company repository timed out after 30 seconds");
+      }
      rethrowAsApiError(err);
    }
  });