fix(settings): opens GitHub app settings for org access (#12)

* fix(settings): opens GitHub app settings for org access

* fix(settings): moves focus listener cleanup to component lifecycle

* fix(settings): uses openGitHubUrl wrapper, adds syncing test

* fix(settings): adds clientId validation, URL guard, and missing tests

* fix(settings): surfaces sync failure notification, tests disabled state

Author: wgordon17

src/app/components/settings/SettingsPage.tsx

@@ -3,7 +3,9 @@ import { useNavigate } from "@solidjs/router";
 import { config, updateConfig } from "../../stores/config";
 import { clearAuth } from "../../stores/auth";
 import { clearCache } from "../../stores/cache";
-import { buildAuthorizeUrl, MERGE_ORGS_KEY } from "../../lib/oauth";
+import { pushNotification } from "../../lib/errors";
+import { buildOrgAccessUrl } from "../../lib/oauth";
+import { isSafeGitHubUrl, openGitHubUrl } from "../../lib/url";
 import { fetchOrgs } from "../../services/api";
 import { getClient } from "../../services/github";
 import OrgSelector from "../onboarding/OrgSelector";
@@ -153,14 +155,15 @@ export default function SettingsPage() {
   const [confirmClearCache, setConfirmClearCache] = createSignal(false);
   const [confirmReset, setConfirmReset] = createSignal(false);
   const [cacheClearing, setCacheClearing] = createSignal(false);
-  const [reauthing, setReauthing] = createSignal(false);
+  const [merging, setMerging] = createSignal(false);
   const [notifPermission, setNotifPermission] = createSignal<NotificationPermission>(
     typeof Notification !== "undefined" ? Notification.permission : "default"
   );
 
   // Save indicator
   const [showSaved, setShowSaved] = createSignal(false);
   let saveTimer: ReturnType<typeof setTimeout> | undefined;
+  let pendingFocusHandler: (() => void) | undefined;
 
   function saveWithFeedback(patch: Parameters<typeof updateConfig>[0]) {
     updateConfig(patch);
@@ -169,7 +172,12 @@ export default function SettingsPage() {
     saveTimer = setTimeout(() => setShowSaved(false), 1500);
   }
 
-  onCleanup(() => clearTimeout(saveTimer));
+  onCleanup(() => {
+    clearTimeout(saveTimer);
+    if (pendingFocusHandler) {
+      window.removeEventListener("focus", pendingFocusHandler);
+    }
+  });
 
   // Local copies for org/repo editing (committed on blur/change)
   const [localOrgs, setLocalOrgs] = createSignal<string[]>(config.selectedOrgs);
@@ -191,20 +199,14 @@ export default function SettingsPage() {
     };
     mq.addEventListener("change", handler);
     onCleanup(() => mq.removeEventListener("change", handler));
-
-    // Auto-merge newly accessible orgs after re-auth redirect
-    const shouldMerge = sessionStorage.getItem(MERGE_ORGS_KEY);
-    if (shouldMerge) {
-      sessionStorage.removeItem(MERGE_ORGS_KEY);
-      void mergeNewOrgs();
-    }
   });
 
   // ── Helpers ──────────────────────────────────────────────────────────────
 
   async function mergeNewOrgs() {
     const client = getClient();
     if (!client) return;
+    setMerging(true);
     const snapshot = [...config.selectedOrgs];
     try {
       const allOrgs = await fetchOrgs(client);
@@ -220,16 +222,27 @@ export default function SettingsPage() {
         console.info(`[settings] merged ${newOrgs.length} new org(s)`);
       }
     } catch {
-      // Non-fatal — user can manually manage orgs
+      pushNotification("org-sync", "Failed to sync organizations — try again or manage manually", "warning");
+    } finally {
+      setMerging(false);
     }
   }
 
-  function handleReAuth() {
-    setReauthing(true);
-    // Reset if navigation doesn't happen (e.g., beforeunload dialog blocks redirect)
-    setTimeout(() => setReauthing(false), 3000);
-    sessionStorage.setItem(MERGE_ORGS_KEY, "true");
-    window.location.href = buildAuthorizeUrl({ returnTo: "/settings" });
+  function handleGrantOrgs() {
+    const url = buildOrgAccessUrl();
+    if (!isSafeGitHubUrl(url)) return;
+    openGitHubUrl(url);
+    // Remove any prior focus listener before adding a new one (dedup on rapid clicks)
+    if (pendingFocusHandler) {
+      window.removeEventListener("focus", pendingFocusHandler);
+    }
+    const onFocus = () => {
+      window.removeEventListener("focus", onFocus);
+      pendingFocusHandler = undefined;
+      void mergeNewOrgs();
+    };
+    pendingFocusHandler = onFocus;
+    window.addEventListener("focus", onFocus);
   }
 
   function handleOrgsChange(orgs: string[]) {
@@ -408,16 +421,16 @@ export default function SettingsPage() {
                     Organization Access
                   </p>
                   <p class="text-xs text-gray-500 dark:text-gray-400">
-                    Re-authorize with GitHub to grant access to additional organizations
+                    Manage organization access on GitHub — new orgs sync automatically when you return
                   </p>
                 </div>
                 <button
                   type="button"
-                  onClick={handleReAuth}
-                  disabled={reauthing()}
+                  onClick={handleGrantOrgs}
+                  disabled={merging()}
                   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 disabled:cursor-not-allowed disabled:opacity-40 dark:border-gray-600 dark:bg-gray-700 dark:text-gray-300 dark:hover:bg-gray-600"
                 >
-                  Grant more orgs
+                  {merging() ? "Syncing..." : "Grant more orgs"}
                 </button>
               </div>
             </div>

src/app/lib/oauth.ts

@@ -1,6 +1,5 @@
 export const OAUTH_STATE_KEY = "github-tracker:oauth-state";
 export const OAUTH_RETURN_TO_KEY = "github-tracker:oauth-return-to";
-export const MERGE_ORGS_KEY = "github-tracker:merge-orgs";
 
 export function generateOAuthState(): string {
   const stateBytes = crypto.getRandomValues(new Uint8Array(16));
@@ -36,3 +35,13 @@ export function buildAuthorizeUrl(options?: { returnTo?: string }): string {
   });
   return `https://github.com/login/oauth/authorize?${params.toString()}`;
 }
+
+const VALID_CLIENT_ID_RE = /^[A-Za-z0-9_-]+$/;
+
+export function buildOrgAccessUrl(): string {
+  const clientId = import.meta.env.VITE_GITHUB_CLIENT_ID as string;
+  if (!clientId || !VALID_CLIENT_ID_RE.test(clientId)) {
+    throw new Error("Invalid VITE_GITHUB_CLIENT_ID");
+  }
+  return `https://github.com/settings/connections/applications/${clientId}`;
+}

tests/components/settings/SettingsPage.test.tsx

@@ -41,6 +41,15 @@ vi.mock("../../../src/app/services/api", () => ({
   fetchRepos: vi.fn().mockResolvedValue([]),
 }));
 
+vi.mock("../../../src/app/lib/url", () => ({
+  isSafeGitHubUrl: vi.fn(() => true),
+  openGitHubUrl: vi.fn(),
+}));
+
+vi.mock("../../../src/app/lib/errors", () => ({
+  pushNotification: vi.fn(),
+}));
+
 // ── Imports after mocks ───────────────────────────────────────────────────────
 
 import { render } from "@solidjs/testing-library";
@@ -50,7 +59,8 @@ import * as authStore from "../../../src/app/stores/auth";
 import * as cacheStore from "../../../src/app/stores/cache";
 import * as apiModule from "../../../src/app/services/api";
 import { updateConfig, config } from "../../../src/app/stores/config";
-import { MERGE_ORGS_KEY, OAUTH_STATE_KEY } from "../../../src/app/lib/oauth";
+import { buildOrgAccessUrl } from "../../../src/app/lib/oauth";
+import * as urlModule from "../../../src/app/lib/url";
 
 // ── Helpers ───────────────────────────────────────────────────────────────────
 
@@ -89,6 +99,9 @@ beforeEach(() => {
   setupMatchMedia();
   vi.clearAllMocks();
 
+  // Restore isSafeGitHubUrl mock (vi.restoreAllMocks strips factory implementations)
+  vi.mocked(urlModule.isSafeGitHubUrl).mockReturnValue(true);
+
   // Reset config to defaults
   updateConfig({
     refreshInterval: 300,
@@ -104,7 +117,6 @@ beforeEach(() => {
     selectedRepos: [],
   });
 
-  // Clear sessionStorage — re-auth tests set MERGE_ORGS_KEY and OAUTH_STATE_KEY
   sessionStorage.clear();
 
   // Mock window.location with both reload and href
@@ -521,126 +533,147 @@ describe("SettingsPage — Theme application", () => {
   });
 });
 
-describe("SettingsPage — Re-auth button", () => {
+describe("SettingsPage — Grant more orgs button", () => {
+  beforeEach(() => {
+    vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
+  });
+  afterEach(() => {
+    vi.unstubAllEnvs();
+  });
+
   it("renders 'Grant more orgs' button in Organizations & Repositories section", () => {
     renderSettings();
     screen.getByRole("button", { name: "Grant more orgs" });
   });
 
-  it("clicking 'Grant more orgs' sets MERGE_ORGS_KEY in sessionStorage", async () => {
+  it("clicking 'Grant more orgs' opens GitHub app settings via openGitHubUrl", async () => {
     const user = userEvent.setup();
-    vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
     renderSettings();
     const btn = screen.getByRole("button", { name: "Grant more orgs" });
     await user.click(btn);
-    expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBe("true");
-    vi.unstubAllEnvs();
+    expect(urlModule.openGitHubUrl).toHaveBeenCalledWith(buildOrgAccessUrl());
   });
 
-  it("clicking 'Grant more orgs' sets window.location.href to GitHub authorize URL", async () => {
+  it("clicking 'Grant more orgs' registers a focus listener for auto-merge", async () => {
     const user = userEvent.setup();
-    vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
+    const addSpy = vi.spyOn(window, "addEventListener");
     renderSettings();
     const btn = screen.getByRole("button", { name: "Grant more orgs" });
     await user.click(btn);
-    expect(window.location.href).toContain("https://github.com/login/oauth/authorize");
-    vi.unstubAllEnvs();
+    expect(addSpy).toHaveBeenCalledWith("focus", expect.any(Function));
   });
 
-  it("clicking 'Grant more orgs' also sets OAUTH_STATE_KEY in sessionStorage", async () => {
+  it("shows disabled 'Syncing...' button during merge, reverts after", async () => {
     const user = userEvent.setup();
-    vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
-    renderSettings();
-    const btn = screen.getByRole("button", { name: "Grant more orgs" });
-    await user.click(btn);
-    expect(sessionStorage.getItem(OAUTH_STATE_KEY)).toBeTruthy();
-    vi.unstubAllEnvs();
-  });
-
-  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");
+    updateConfig({ selectedOrgs: [] });
+    let resolveFetch!: (v: never[]) => void;
+    vi.mocked(apiModule.fetchOrgs).mockReturnValue(
+      new Promise((r) => { resolveFetch = r as (v: never[]) => void; })
+    );
     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();
+    window.dispatchEvent(new Event("focus"));
+    await waitFor(() => {
+      const syncBtn = screen.getByRole("button", { name: "Syncing..." });
+      expect(syncBtn.hasAttribute("disabled")).toBe(true);
+    });
+    resolveFetch([]);
+    await waitFor(() => {
+      const restored = screen.getByRole("button", { name: "Grant more orgs" });
+      expect(restored.hasAttribute("disabled")).toBe(false);
+    });
   });
-});
 
-describe("SettingsPage — Auto-merge orgs on mount", () => {
-  it("calls fetchOrgs and merges new orgs when MERGE_ORGS_KEY is in sessionStorage", async () => {
-    sessionStorage.setItem(MERGE_ORGS_KEY, "true");
+  it("auto-merges new orgs when window regains focus after granting", async () => {
+    const user = userEvent.setup();
     updateConfig({ selectedOrgs: ["existing-org"] });
     vi.mocked(apiModule.fetchOrgs).mockResolvedValue([
       { login: "existing-org", avatarUrl: "", type: "org" },
       { login: "new-org", avatarUrl: "", type: "org" },
     ]);
     renderSettings();
+    const btn = screen.getByRole("button", { name: "Grant more orgs" });
+    await user.click(btn);
+    window.dispatchEvent(new Event("focus"));
     await waitFor(() => {
       expect(apiModule.fetchOrgs).toHaveBeenCalled();
     });
     await waitFor(() => {
       expect(config.selectedOrgs).toContain("new-org");
+      expect(config.selectedOrgs).toContain("existing-org");
     });
   });
 
-  it("preserves existing selectedOrgs when merging", async () => {
-    sessionStorage.setItem(MERGE_ORGS_KEY, "true");
+  it("pushes warning notification on fetchOrgs failure", async () => {
+    const { pushNotification } = await import("../../../src/app/lib/errors");
+    const user = userEvent.setup();
     updateConfig({ selectedOrgs: ["existing-org"] });
-    vi.mocked(apiModule.fetchOrgs).mockResolvedValue([
-      { login: "existing-org", avatarUrl: "", type: "org" },
-      { login: "new-org", avatarUrl: "", type: "org" },
-    ]);
+    vi.mocked(apiModule.fetchOrgs).mockRejectedValue(new Error("Network error"));
     renderSettings();
+    const btn = screen.getByRole("button", { name: "Grant more orgs" });
+    await user.click(btn);
+    window.dispatchEvent(new Event("focus"));
     await waitFor(() => {
-      expect(config.selectedOrgs).toContain("existing-org");
+      expect(apiModule.fetchOrgs).toHaveBeenCalled();
     });
-  });
-
-  it("removes MERGE_ORGS_KEY from sessionStorage after processing", async () => {
-    sessionStorage.setItem(MERGE_ORGS_KEY, "true");
-    updateConfig({ selectedOrgs: [] });
-    vi.mocked(apiModule.fetchOrgs).mockResolvedValue([
-      { login: "new-org", avatarUrl: "", type: "org" },
-    ]);
-    renderSettings();
     await waitFor(() => {
-      expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBeNull();
+      expect(pushNotification).toHaveBeenCalledWith(
+        "org-sync",
+        expect.stringContaining("Failed to sync"),
+        "warning",
+      );
     });
+    expect(config.selectedOrgs).toEqual(["existing-org"]);
   });
 
-  it("does not call fetchOrgs when MERGE_ORGS_KEY is not in sessionStorage", () => {
-    // No MERGE_ORGS_KEY set
+  it("skips merge on focus when getClient returns null", async () => {
+    const user = userEvent.setup();
+    const github = await import("../../../src/app/services/github");
+    vi.mocked(github.getClient).mockReturnValueOnce(null);
     renderSettings();
+    const btn = screen.getByRole("button", { name: "Grant more orgs" });
+    await user.click(btn);
+    window.dispatchEvent(new Event("focus"));
+    await new Promise((r) => setTimeout(r, 50));
     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"));
+  it("rapid double-click deduplicates focus listeners", async () => {
+    const user = userEvent.setup();
+    const removeSpy = vi.spyOn(window, "removeEventListener");
     renderSettings();
-    await waitFor(() => {
-      expect(apiModule.fetchOrgs).toHaveBeenCalled();
-    });
-    // Config unchanged — error was swallowed
-    expect(config.selectedOrgs).toEqual(["existing-org"]);
+    const btn = screen.getByRole("button", { name: "Grant more orgs" });
+    await user.click(btn);
+    await user.click(btn);
+    const focusRemoves = removeSpy.mock.calls.filter(([evt]) => evt === "focus");
+    expect(focusRemoves.length).toBeGreaterThanOrEqual(1);
   });
 
-  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);
+  it("cleans up pending focus listener on component unmount", async () => {
+    const user = userEvent.setup();
+    const removeSpy = vi.spyOn(window, "removeEventListener");
+    const { unmount } = renderSettings();
+    const btn = screen.getByRole("button", { name: "Grant more orgs" });
+    await user.click(btn);
+    unmount();
+    const focusRemoves = removeSpy.mock.calls.filter(([evt]) => evt === "focus");
+    expect(focusRemoves.length).toBeGreaterThanOrEqual(1);
+  });
+
+  it("focus listener self-removes — second focus does not re-trigger merge", async () => {
+    const user = userEvent.setup();
+    updateConfig({ selectedOrgs: [] });
+    vi.mocked(apiModule.fetchOrgs).mockResolvedValue([]);
     renderSettings();
-    // MERGE_ORGS_KEY still removed synchronously before mergeNewOrgs
+    const btn = screen.getByRole("button", { name: "Grant more orgs" });
+    await user.click(btn);
+    window.dispatchEvent(new Event("focus"));
     await waitFor(() => {
-      expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBeNull();
+      expect(apiModule.fetchOrgs).toHaveBeenCalledTimes(1);
     });
-    expect(apiModule.fetchOrgs).not.toHaveBeenCalled();
+    window.dispatchEvent(new Event("focus"));
+    await new Promise((r) => setTimeout(r, 50));
+    expect(apiModule.fetchOrgs).toHaveBeenCalledTimes(1);
   });
 });