fix: addresses PR review findings across all domains

wgordon17 · wgordon17 · commit 314c4a5d89f9 · 2026-03-25T07:45:48.000-04:00
diff --git a/src/app/components/onboarding/OnboardingWizard.tsx b/src/app/components/onboarding/OnboardingWizard.tsx
@@ -121,7 +121,7 @@ export default function OnboardingWizard() {
               type="button"
               onClick={handleFinish}
               disabled={selectedRepos().length === 0}
-              class="ml-auto rounded-md bg-blue-600 px-4 py-2 text-sm font-medium text-white hover:bg-blue-700 disabled:cursor-not-allowed disabled:opacity-40 dark:bg-blue-500 dark:hover:bg-blue-600"
+              class="rounded-md bg-blue-600 px-4 py-2 text-sm font-medium text-white hover:bg-blue-700 disabled:cursor-not-allowed disabled:opacity-40 dark:bg-blue-500 dark:hover:bg-blue-600"
             >
               {selectedRepos().length === 0
                 ? "Finish Setup"
diff --git a/src/app/components/onboarding/RepoSelector.tsx b/src/app/components/onboarding/RepoSelector.tsx
@@ -132,6 +132,8 @@ export default function RepoSelector(props: RepoSelectorProps) {
     const client = getClient();
     if (!client) return;
 
+    const version = effectVersion;
+
     setOrgStates((prev) =>
       prev.map((s) =>
         s.org === org ? { ...s, loading: true, error: null } : s
@@ -143,13 +145,15 @@ export default function RepoSelector(props: RepoSelectorProps) {
 
     void fetchRepos(client, org, type)
       .then((repos) => {
+        if (version !== effectVersion) return;
         setOrgStates((prev) =>
           prev.map((s) =>
             s.org === org ? { ...s, repos, loading: false, error: null } : s
           )
         );
       })
       .catch((err) => {
+        if (version !== effectVersion) return;
         const message =
           err instanceof Error ? err.message : "Failed to load repositories";
         setOrgStates((prev) =>
@@ -370,7 +374,6 @@ export default function RepoSelector(props: RepoSelectorProps) {
                     class="max-h-[300px] overflow-y-auto"
                     role="region"
                     aria-label={`${state().org} repositories`}
-                    data-testid={`repo-scroll-${state().org}`}
                   >
                     <ul class="divide-y divide-gray-100 dark:divide-gray-700">
                       <Index each={visible()}>
diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx
@@ -205,18 +205,19 @@ export default function SettingsPage() {
   async function mergeNewOrgs() {
     const client = getClient();
     if (!client) return;
+    const snapshot = [...config.selectedOrgs];
     try {
       const allOrgs = await fetchOrgs(client);
       // Case-insensitive comparison — GitHub logins are case-insensitive
-      const currentSet = new Set(config.selectedOrgs.map((o) => o.toLowerCase()));
+      const currentSet = new Set(snapshot.map((o) => o.toLowerCase()));
       const newOrgs = allOrgs
         .map((o) => o.login)
         .filter((login) => !currentSet.has(login.toLowerCase()));
       if (newOrgs.length > 0) {
-        const merged = [...config.selectedOrgs, ...newOrgs];
+        const merged = [...snapshot, ...newOrgs];
         setLocalOrgs(merged);
         saveWithFeedback({ selectedOrgs: merged });
-        console.info(`[settings] merged ${newOrgs.length} new org(s): ${newOrgs.join(", ")}`);
+        console.info(`[settings] merged ${newOrgs.length} new org(s)`);
       }
     } catch {
       // Non-fatal — user can manually manage orgs
@@ -421,25 +422,27 @@ export default function SettingsPage() {
               </div>
             </div>
 
-            <div class="border-t border-gray-100 pt-3 dark:border-gray-700 flex items-center justify-between">
-              <div>
-                <p class="text-sm font-medium text-gray-900 dark:text-gray-100">Repositories</p>
-                <p class="text-xs text-gray-500 dark:text-gray-400">
-                  {localRepos().length} selected
-                </p>
-                <Show when={localRepos().length > 50}>
-                  <p class="text-xs text-amber-600 dark:text-amber-400">
-                    Tracking {localRepos().length} repos will use significant API quota per poll cycle
+            <div class="border-t border-gray-100 pt-3 dark:border-gray-700">
+              <div class="flex items-center justify-between">
+                <div>
+                  <p class="text-sm font-medium text-gray-900 dark:text-gray-100">Repositories</p>
+                  <p class="text-xs text-gray-500 dark:text-gray-400">
+                    {localRepos().length} selected
                   </p>
-                </Show>
+                  <Show when={localRepos().length > 50}>
+                    <p class="text-xs text-amber-600 dark:text-amber-400">
+                      Tracking {localRepos().length} repos will use significant API quota per poll cycle
+                    </p>
+                  </Show>
+                </div>
+                <button
+                  type="button"
+                  onClick={() => setRepoPanelOpen((v) => !v)}
+                  class="rounded-md border border-gray-300 bg-white px-3 py-1.5 text-sm font-medium text-gray-700 hover:bg-gray-50 dark:border-gray-600 dark:bg-gray-700 dark:text-gray-300 dark:hover:bg-gray-600"
+                >
+                  Manage Repositories
+                </button>
               </div>
-              <button
-                type="button"
-                onClick={() => setRepoPanelOpen((v) => !v)}
-                class="rounded-md border border-gray-300 bg-white px-3 py-1.5 text-sm font-medium text-gray-700 hover:bg-gray-50 dark:border-gray-600 dark:bg-gray-700 dark:text-gray-300 dark:hover:bg-gray-600"
-              >
-                Manage Repositories
-              </button>
             </div>
             <Show when={repoPanelOpen()}>
               <div class="rounded-lg border border-gray-200 p-4 dark:border-gray-700">
diff --git a/src/app/lib/oauth.ts b/src/app/lib/oauth.ts
@@ -10,6 +10,12 @@ export function generateOAuthState(): string {
     .replace(/=/g, "");
 }
 
+export function sanitizeReturnTo(returnTo: string | null): string {
+  return returnTo && returnTo.startsWith("/") && !returnTo.startsWith("//")
+    ? returnTo
+    : "/";
+}
+
 export function buildAuthorizeUrl(options?: { returnTo?: string }): string {
   const clientId = import.meta.env.VITE_GITHUB_CLIENT_ID as string;
   const state = generateOAuthState();
diff --git a/src/app/pages/OAuthCallback.tsx b/src/app/pages/OAuthCallback.tsx
@@ -1,7 +1,8 @@
 import { createSignal, onMount, Show } from "solid-js";
 import { useNavigate } from "@solidjs/router";
 import { setAuth, validateToken, clearAuth } from "../stores/auth";
-import { OAUTH_STATE_KEY, OAUTH_RETURN_TO_KEY } from "../lib/oauth";
+import { OAUTH_STATE_KEY, OAUTH_RETURN_TO_KEY, sanitizeReturnTo } from "../lib/oauth";
+import LoadingSpinner from "../components/shared/LoadingSpinner";
 
 interface TokenResponse {
   access_token: string;
@@ -63,12 +64,7 @@ export default function OAuthCallback() {
         return;
       }
 
-      // Only allow internal paths (prevent open redirect)
-      const target =
-        returnTo && returnTo.startsWith("/") && !returnTo.startsWith("//")
-          ? returnTo
-          : "/";
-      navigate(target, { replace: true });
+      navigate(sanitizeReturnTo(returnTo), { replace: true });
     } catch {
       setError("A network error occurred. Please try again.");
     }
@@ -81,30 +77,7 @@ export default function OAuthCallback() {
           when={error()}
           fallback={
             <div class="bg-white dark:bg-gray-800 rounded-xl shadow-md p-8 flex flex-col items-center gap-4">
-              <svg
-                class="animate-spin h-8 w-8 text-gray-500"
-                xmlns="http://www.w3.org/2000/svg"
-                fill="none"
-                viewBox="0 0 24 24"
-                aria-label="Loading"
-              >
-                <circle
-                  class="opacity-25"
-                  cx="12"
-                  cy="12"
-                  r="10"
-                  stroke="currentColor"
-                  stroke-width="4"
-                />
-                <path
-                  class="opacity-75"
-                  fill="currentColor"
-                  d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4z"
-                />
-              </svg>
-              <p class="text-gray-600 dark:text-gray-400">
-                Completing sign in...
-              </p>
+              <LoadingSpinner size="md" label="Completing sign in..." />
             </div>
           }
         >
diff --git a/tests/components/OAuthCallback.test.tsx b/tests/components/OAuthCallback.test.tsx
@@ -373,26 +373,6 @@ describe("OAuthCallback", () => {
     expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull();
   });
 
-  // Verify the returnTo validation logic directly — the navigate() call itself
-  // cannot be intercepted due to SolidJS router limitations (partial mock of
-  // @solidjs/router renders empty div — project gotcha), but we can verify
-  // the logic by testing the validation conditions match the implementation.
-  it("returnTo validation accepts internal paths and rejects external URLs", () => {
-    // This tests the SAME validation logic used on OAuthCallback line 67-70:
-    // returnTo && returnTo.startsWith("/") && !returnTo.startsWith("//")
-    const validate = (returnTo: string | null) =>
-      returnTo && returnTo.startsWith("/") && !returnTo.startsWith("//")
-        ? returnTo
-        : "/";
-
-    expect(validate("/settings")).toBe("/settings");
-    expect(validate("/dashboard")).toBe("/dashboard");
-    expect(validate("/")).toBe("/");
-    expect(validate("https://evil.com")).toBe("/");
-    expect(validate("//evil.com")).toBe("/");
-    expect(validate("javascript:alert(1)")).toBe("/");
-    expect(validate(null)).toBe("/");
-    expect(validate("")).toBe("/");
-  });
+  // returnTo validation is tested via sanitizeReturnTo in tests/lib/oauth.test.ts
 
 });
diff --git a/tests/components/onboarding/OnboardingWizard.test.tsx b/tests/components/onboarding/OnboardingWizard.test.tsx
@@ -117,7 +117,42 @@ describe("OnboardingWizard", () => {
     vi.mocked(apiModule.fetchOrgs).mockReturnValue(new Promise(() => {}));
     render(() => <OnboardingWizard />);
     await waitFor(() => {
-      expect(screen.getByTestId("loading-spinner")).toBeDefined();
+      screen.getByTestId("loading-spinner");
+    });
+  });
+
+  it("redirects to /dashboard when onboardingComplete is already true", async () => {
+    Object.assign(configStore.config, { onboardingComplete: true });
+    render(() => <OnboardingWizard />);
+    await waitFor(() => {
+      expect(window.location.replace).toHaveBeenCalledWith("/dashboard");
+    });
+    expect(apiModule.fetchOrgs).not.toHaveBeenCalled();
+    Object.assign(configStore.config, { onboardingComplete: false });
+  });
+
+  it("retry clears error and shows RepoSelector on success", async () => {
+    const user = userEvent.setup();
+    vi.mocked(apiModule.fetchOrgs)
+      .mockRejectedValueOnce(new Error("Network error"))
+      .mockResolvedValueOnce(mockOrgs);
+    render(() => <OnboardingWizard />);
+    await waitFor(() => {
+      screen.getByText("Retry");
+    });
+    await user.click(screen.getByText("Retry"));
+    await waitFor(() => {
+      screen.getByTestId("repo-selector");
+    });
+    expect(screen.queryByText(/Network error/i)).toBeNull();
+  });
+
+  it("shows error when getClient returns null", async () => {
+    const { getClient } = await import("../../../src/app/services/github");
+    vi.mocked(getClient).mockReturnValueOnce(null);
+    render(() => <OnboardingWizard />);
+    await waitFor(() => {
+      screen.getByText(/No GitHub client available/i);
     });
   });
 
diff --git a/tests/components/onboarding/RepoSelector.test.tsx b/tests/components/onboarding/RepoSelector.test.tsx
@@ -313,7 +313,7 @@ describe("RepoSelector", () => {
     expect(orgHeaders[1].textContent).toBe("active-org");
   });
 
-  it("each org group has a scroll container with data-testid=repo-scroll-{org}", async () => {
+  it("each org group has a scrollable region with aria-label", async () => {
     vi.mocked(api.fetchRepos).mockImplementation((_client, org) => {
       if (org === "myorg") return Promise.resolve(myorgRepos);
       return Promise.resolve(otherorgRepos);
@@ -325,8 +325,8 @@ describe("RepoSelector", () => {
       screen.getByText("repo-a");
       screen.getByText("repo-c");
     });
-    expect(screen.getByTestId("repo-scroll-myorg")).toBeDefined();
-    expect(screen.getByTestId("repo-scroll-otherog")).toBeDefined();
+    screen.getByRole("region", { name: "myorg repositories" });
+    screen.getByRole("region", { name: "otherog repositories" });
   });
 
   it("scroll container has max-h-[300px] and overflow-y-auto classes", async () => {
@@ -337,8 +337,28 @@ describe("RepoSelector", () => {
     await waitFor(() => {
       screen.getByText("repo-a");
     });
-    const scrollContainer = screen.getByTestId("repo-scroll-myorg");
+    const scrollContainer = screen.getByRole("region", { name: "myorg repositories" });
     expect(scrollContainer.classList.contains("max-h-[300px]")).toBe(true);
     expect(scrollContainer.classList.contains("overflow-y-auto")).toBe(true);
   });
+
+  it("skips internal fetchOrgs when orgEntries prop is provided", async () => {
+    vi.mocked(api.fetchOrgs).mockClear();
+    vi.mocked(api.fetchRepos).mockResolvedValue(myorgRepos);
+    const preloaded = [
+      { login: "myorg", avatarUrl: "", type: "org" as const },
+    ];
+    render(() => (
+      <RepoSelector
+        selectedOrgs={["myorg"]}
+        orgEntries={preloaded}
+        selected={[]}
+        onChange={vi.fn()}
+      />
+    ));
+    await waitFor(() => {
+      screen.getByText("repo-a");
+    });
+    expect(api.fetchOrgs).not.toHaveBeenCalled();
+  });
 });
diff --git a/tests/components/settings/SettingsPage.test.tsx b/tests/components/settings/SettingsPage.test.tsx
@@ -33,7 +33,7 @@ vi.mock("../../../src/app/stores/cache", () => ({
 }));
 
 vi.mock("../../../src/app/services/github", () => ({
-  getClient: () => ({}),
+  getClient: vi.fn(() => ({})),
 }));
 
 vi.mock("../../../src/app/services/api", () => ({
@@ -143,7 +143,6 @@ describe("SettingsPage — rendering", () => {
   it("renders a back to dashboard link", () => {
     renderSettings();
     const backLink = screen.getByRole("link", { name: /back to dashboard/i });
-    expect(backLink).toBeDefined();
     expect(backLink.getAttribute("href")).toBe("/dashboard");
   });
 
@@ -193,8 +192,7 @@ describe("SettingsPage — rendering", () => {
 describe("SettingsPage — Refresh interval", () => {
   it("shows current refresh interval value", () => {
     renderSettings();
-    const select = screen.getByDisplayValue("5 minutes (default)");
-    expect(select).toBeDefined();
+    screen.getByDisplayValue("5 minutes (default)");
   });
 
   it("changing refresh interval calls updateConfig", async () => {
@@ -284,6 +282,8 @@ describe("SettingsPage — GitHub Actions", () => {
     expect(workflowInput).toBeDefined();
   });
 
+  // NumberInput uses onInput — fireEvent.input sets the value atomically, while
+  // userEvent.type fires per-keystroke triggering intermediate valid values.
   it("changing max workflows per repo updates config", () => {
     renderSettings();
     const inputs = screen.getAllByRole("spinbutton");
@@ -557,14 +557,18 @@ describe("SettingsPage — Re-auth button", () => {
     vi.unstubAllEnvs();
   });
 
-  it("'Grant more orgs' button is disabled after click (reentrancy guard)", async () => {
-    const user = userEvent.setup();
+  it("'Grant more orgs' button is disabled after click and re-enables after timeout", async () => {
+    vi.useFakeTimers();
+    const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime });
     vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
     renderSettings();
     const btn = screen.getByRole("button", { name: "Grant more orgs" });
     await user.click(btn);
     expect(btn.hasAttribute("disabled")).toBe(true);
+    vi.advanceTimersByTime(3000);
+    expect(btn.hasAttribute("disabled")).toBe(false);
     vi.unstubAllEnvs();
+    vi.useRealTimers();
   });
 });
 
@@ -615,4 +619,28 @@ describe("SettingsPage — Auto-merge orgs on mount", () => {
     renderSettings();
     expect(apiModule.fetchOrgs).not.toHaveBeenCalled();
   });
+
+  it("silently handles fetchOrgs rejection without breaking", async () => {
+    sessionStorage.setItem(MERGE_ORGS_KEY, "true");
+    updateConfig({ selectedOrgs: ["existing-org"] });
+    vi.mocked(apiModule.fetchOrgs).mockRejectedValue(new Error("Network error"));
+    renderSettings();
+    await waitFor(() => {
+      expect(apiModule.fetchOrgs).toHaveBeenCalled();
+    });
+    // Config unchanged — error was swallowed
+    expect(config.selectedOrgs).toEqual(["existing-org"]);
+  });
+
+  it("skips merge when getClient returns null", async () => {
+    sessionStorage.setItem(MERGE_ORGS_KEY, "true");
+    const github = await import("../../../src/app/services/github");
+    vi.mocked(github.getClient).mockReturnValueOnce(null);
+    renderSettings();
+    // MERGE_ORGS_KEY still removed synchronously before mergeNewOrgs
+    await waitFor(() => {
+      expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBeNull();
+    });
+    expect(apiModule.fetchOrgs).not.toHaveBeenCalled();
+  });
 });
diff --git a/tests/lib/oauth.test.ts b/tests/lib/oauth.test.ts

Original file line number	Diff line number	Diff line change
`@@ -121,7 +121,7 @@ export default function OnboardingWizard() {`
`121`	`121`	`type="button"`
`122`	`122`	`onClick={handleFinish}`
`123`	`123`	`disabled={selectedRepos().length === 0}`
`124`		`- class="ml-auto rounded-md bg-blue-600 px-4 py-2 text-sm font-medium text-white hover:bg-blue-700 disabled:cursor-not-allowed disabled:opacity-40 dark:bg-blue-500 dark:hover:bg-blue-600"`
	`124`	`+ class="rounded-md bg-blue-600 px-4 py-2 text-sm font-medium text-white hover:bg-blue-700 disabled:cursor-not-allowed disabled:opacity-40 dark:bg-blue-500 dark:hover:bg-blue-600"`
`125`	`125`	`>`
`126`	`126`	`{selectedRepos().length === 0`
`127`	`127`	`? "Finish Setup"`