From ed386dc5489db5bf6e543e668dc70a1ea19abf60 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 25 Mar 2026 11:09:35 -0400 Subject: [PATCH 1/5] fix(settings): opens GitHub app settings for org access --- src/app/components/settings/SettingsPage.tsx | 37 +++--- src/app/lib/oauth.ts | 6 +- .../components/settings/SettingsPage.test.tsx | 112 ++++++------------ tests/lib/oauth.test.ts | 10 ++ 4 files changed, 72 insertions(+), 93 deletions(-) diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index c9a0affb..93d34533 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -3,7 +3,7 @@ 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 { buildOrgAccessUrl } from "../../lib/oauth"; import { fetchOrgs } from "../../services/api"; import { getClient } from "../../services/github"; import OrgSelector from "../onboarding/OrgSelector"; @@ -153,7 +153,7 @@ 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( typeof Notification !== "undefined" ? Notification.permission : "default" ); @@ -191,13 +191,6 @@ 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 ────────────────────────────────────────────────────────────── @@ -205,6 +198,7 @@ export default function SettingsPage() { async function mergeNewOrgs() { const client = getClient(); if (!client) return; + setMerging(true); const snapshot = [...config.selectedOrgs]; try { const allOrgs = await fetchOrgs(client); @@ -221,15 +215,20 @@ export default function SettingsPage() { } } catch { // Non-fatal — user can manually manage orgs + } 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() { + window.open(buildOrgAccessUrl(), "_blank", "noopener"); + // Auto-merge newly accessible orgs when user returns from GitHub settings + const onFocus = () => { + window.removeEventListener("focus", onFocus); + void mergeNewOrgs(); + }; + window.addEventListener("focus", onFocus); + onCleanup(() => window.removeEventListener("focus", onFocus)); } function handleOrgsChange(orgs: string[]) { @@ -408,16 +407,16 @@ export default function SettingsPage() { Organization Access

- Re-authorize with GitHub to grant access to additional organizations + Manage organization access on GitHub — new orgs sync automatically when you return

diff --git a/src/app/lib/oauth.ts b/src/app/lib/oauth.ts index decf5175..e1f901fd 100644 --- a/src/app/lib/oauth.ts +++ b/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,8 @@ export function buildAuthorizeUrl(options?: { returnTo?: string }): string { }); return `https://github.com/login/oauth/authorize?${params.toString()}`; } + +export function buildOrgAccessUrl(): string { + const clientId = import.meta.env.VITE_GITHUB_CLIENT_ID as string; + return `https://github.com/settings/connections/applications/${clientId}`; +} diff --git a/tests/components/settings/SettingsPage.test.tsx b/tests/components/settings/SettingsPage.test.tsx index 728a2fac..de6a69f3 100644 --- a/tests/components/settings/SettingsPage.test.tsx +++ b/tests/components/settings/SettingsPage.test.tsx @@ -50,7 +50,7 @@ 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"; // ── Helpers ─────────────────────────────────────────────────────────────────── @@ -104,7 +104,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 +520,93 @@ describe("SettingsPage — Theme application", () => { }); }); -describe("SettingsPage — Re-auth button", () => { +describe("SettingsPage — Grant more orgs button", () => { 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 in new tab", async () => { const user = userEvent.setup(); vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + const openSpy = vi.spyOn(window, "open").mockImplementation(() => null); renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); await user.click(btn); - expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBe("true"); + expect(openSpy).toHaveBeenCalledWith( + buildOrgAccessUrl(), + "_blank", + "noopener", + ); vi.unstubAllEnvs(); }); - 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"); + vi.spyOn(window, "open").mockImplementation(() => null); + 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"); + expect(addSpy).toHaveBeenCalledWith("focus", expect.any(Function)); vi.unstubAllEnvs(); }); - it("clicking 'Grant more orgs' also sets OAUTH_STATE_KEY in sessionStorage", async () => { + it("auto-merges new orgs when window regains focus after granting", 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"); - 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(); - }); -}); - -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"); + vi.spyOn(window, "open").mockImplementation(() => null); 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); + // Simulate user returning from GitHub settings tab + window.dispatchEvent(new Event("focus")); await waitFor(() => { expect(apiModule.fetchOrgs).toHaveBeenCalled(); }); await waitFor(() => { expect(config.selectedOrgs).toContain("new-org"); - }); - }); - - it("preserves existing selectedOrgs when merging", async () => { - sessionStorage.setItem(MERGE_ORGS_KEY, "true"); - updateConfig({ selectedOrgs: ["existing-org"] }); - vi.mocked(apiModule.fetchOrgs).mockResolvedValue([ - { login: "existing-org", avatarUrl: "", type: "org" }, - { login: "new-org", avatarUrl: "", type: "org" }, - ]); - renderSettings(); - await waitFor(() => { expect(config.selectedOrgs).toContain("existing-org"); }); + vi.unstubAllEnvs(); }); - 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(); - }); - }); - - it("does not call fetchOrgs when MERGE_ORGS_KEY is not in sessionStorage", () => { - // No MERGE_ORGS_KEY set - renderSettings(); - expect(apiModule.fetchOrgs).not.toHaveBeenCalled(); - }); - - it("silently handles fetchOrgs rejection without breaking", async () => { - sessionStorage.setItem(MERGE_ORGS_KEY, "true"); + it("silently handles fetchOrgs rejection on focus without breaking", async () => { + const user = userEvent.setup(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + vi.spyOn(window, "open").mockImplementation(() => null); updateConfig({ selectedOrgs: ["existing-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(apiModule.fetchOrgs).toHaveBeenCalled(); }); - // Config unchanged — error was swallowed expect(config.selectedOrgs).toEqual(["existing-org"]); + vi.unstubAllEnvs(); }); - it("skips merge when getClient returns null", async () => { - sessionStorage.setItem(MERGE_ORGS_KEY, "true"); + it("skips merge on focus when getClient returns null", async () => { + const user = userEvent.setup(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + vi.spyOn(window, "open").mockImplementation(() => null); const github = await import("../../../src/app/services/github"); - vi.mocked(github.getClient).mockReturnValueOnce(null); + vi.mocked(github.getClient).mockReturnValue(null); renderSettings(); - // MERGE_ORGS_KEY still removed synchronously before mergeNewOrgs - await waitFor(() => { - expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBeNull(); - }); + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + window.dispatchEvent(new Event("focus")); + // Give mergeNewOrgs time to bail out + await new Promise((r) => setTimeout(r, 50)); expect(apiModule.fetchOrgs).not.toHaveBeenCalled(); + vi.unstubAllEnvs(); }); }); diff --git a/tests/lib/oauth.test.ts b/tests/lib/oauth.test.ts index 89c6ebe6..9d7706c2 100644 --- a/tests/lib/oauth.test.ts +++ b/tests/lib/oauth.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { generateOAuthState, buildAuthorizeUrl, + buildOrgAccessUrl, sanitizeReturnTo, OAUTH_STATE_KEY, OAUTH_RETURN_TO_KEY, @@ -97,6 +98,15 @@ describe("oauth helpers", () => { }); }); + describe("buildOrgAccessUrl", () => { + it("returns GitHub connections URL with client ID", () => { + const url = buildOrgAccessUrl(); + expect(url).toBe( + "https://github.com/settings/connections/applications/test-client-id" + ); + }); + }); + describe("sanitizeReturnTo", () => { it("accepts internal paths", () => { expect(sanitizeReturnTo("/settings")).toBe("/settings"); From 2ac948b1d116ce13a66d79b27cecced89971fb79 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 25 Mar 2026 11:19:18 -0400 Subject: [PATCH 2/5] fix(settings): moves focus listener cleanup to component lifecycle --- src/app/components/settings/SettingsPage.tsx | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index 93d34533..a95c5515 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -161,6 +161,7 @@ export default function SettingsPage() { // Save indicator const [showSaved, setShowSaved] = createSignal(false); let saveTimer: ReturnType | undefined; + let pendingFocusHandler: (() => void) | undefined; function saveWithFeedback(patch: Parameters[0]) { updateConfig(patch); @@ -169,7 +170,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(config.selectedOrgs); @@ -222,13 +228,17 @@ export default function SettingsPage() { function handleGrantOrgs() { window.open(buildOrgAccessUrl(), "_blank", "noopener"); - // Auto-merge newly accessible orgs when user returns from GitHub settings + // 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); - onCleanup(() => window.removeEventListener("focus", onFocus)); } function handleOrgsChange(orgs: string[]) { From 08f0124d6622e86284ca878cc669d5cc226d7b68 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 25 Mar 2026 11:27:03 -0400 Subject: [PATCH 3/5] fix(settings): uses openGitHubUrl wrapper, adds syncing test --- src/app/components/settings/SettingsPage.tsx | 3 +- .../components/settings/SettingsPage.test.tsx | 36 +++++++++++++------ 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index a95c5515..83df33d6 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -4,6 +4,7 @@ import { config, updateConfig } from "../../stores/config"; import { clearAuth } from "../../stores/auth"; import { clearCache } from "../../stores/cache"; import { buildOrgAccessUrl } from "../../lib/oauth"; +import { openGitHubUrl } from "../../lib/url"; import { fetchOrgs } from "../../services/api"; import { getClient } from "../../services/github"; import OrgSelector from "../onboarding/OrgSelector"; @@ -227,7 +228,7 @@ export default function SettingsPage() { } function handleGrantOrgs() { - window.open(buildOrgAccessUrl(), "_blank", "noopener"); + openGitHubUrl(buildOrgAccessUrl()); // Remove any prior focus listener before adding a new one (dedup on rapid clicks) if (pendingFocusHandler) { window.removeEventListener("focus", pendingFocusHandler); diff --git a/tests/components/settings/SettingsPage.test.tsx b/tests/components/settings/SettingsPage.test.tsx index de6a69f3..cf76fafe 100644 --- a/tests/components/settings/SettingsPage.test.tsx +++ b/tests/components/settings/SettingsPage.test.tsx @@ -41,6 +41,10 @@ vi.mock("../../../src/app/services/api", () => ({ fetchRepos: vi.fn().mockResolvedValue([]), })); +vi.mock("../../../src/app/lib/url", () => ({ + openGitHubUrl: vi.fn(), +})); + // ── Imports after mocks ─────────────────────────────────────────────────────── import { render } from "@solidjs/testing-library"; @@ -51,6 +55,7 @@ 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 { buildOrgAccessUrl } from "../../../src/app/lib/oauth"; +import * as urlModule from "../../../src/app/lib/url"; // ── Helpers ─────────────────────────────────────────────────────────────────── @@ -526,25 +531,19 @@ describe("SettingsPage — Grant more orgs button", () => { screen.getByRole("button", { name: "Grant more orgs" }); }); - it("clicking 'Grant more orgs' opens GitHub app settings in new tab", 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"); - const openSpy = vi.spyOn(window, "open").mockImplementation(() => null); renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); await user.click(btn); - expect(openSpy).toHaveBeenCalledWith( - buildOrgAccessUrl(), - "_blank", - "noopener", - ); + expect(urlModule.openGitHubUrl).toHaveBeenCalledWith(buildOrgAccessUrl()); vi.unstubAllEnvs(); }); 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"); - vi.spyOn(window, "open").mockImplementation(() => null); const addSpy = vi.spyOn(window, "addEventListener"); renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); @@ -553,10 +552,27 @@ describe("SettingsPage — Grant more orgs button", () => { vi.unstubAllEnvs(); }); + it("shows 'Syncing...' on button while merging and reverts after", async () => { + const user = userEvent.setup(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + updateConfig({ selectedOrgs: [] }); + vi.mocked(apiModule.fetchOrgs).mockResolvedValue([]); + renderSettings(); + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + window.dispatchEvent(new Event("focus")); + await waitFor(() => { + expect(screen.getByRole("button", { name: "Syncing..." })).toBeDefined(); + }); + await waitFor(() => { + expect(screen.getByRole("button", { name: "Grant more orgs" })).toBeDefined(); + }); + vi.unstubAllEnvs(); + }); + it("auto-merges new orgs when window regains focus after granting", async () => { const user = userEvent.setup(); vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); - vi.spyOn(window, "open").mockImplementation(() => null); updateConfig({ selectedOrgs: ["existing-org"] }); vi.mocked(apiModule.fetchOrgs).mockResolvedValue([ { login: "existing-org", avatarUrl: "", type: "org" }, @@ -580,7 +596,6 @@ describe("SettingsPage — Grant more orgs button", () => { it("silently handles fetchOrgs rejection on focus without breaking", async () => { const user = userEvent.setup(); vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); - vi.spyOn(window, "open").mockImplementation(() => null); updateConfig({ selectedOrgs: ["existing-org"] }); vi.mocked(apiModule.fetchOrgs).mockRejectedValue(new Error("Network error")); renderSettings(); @@ -597,7 +612,6 @@ describe("SettingsPage — Grant more orgs button", () => { it("skips merge on focus when getClient returns null", async () => { const user = userEvent.setup(); vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); - vi.spyOn(window, "open").mockImplementation(() => null); const github = await import("../../../src/app/services/github"); vi.mocked(github.getClient).mockReturnValue(null); renderSettings(); From c2268a47ebfe2b17f1b9d8eabdee5c4a0bf5f9ac Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 25 Mar 2026 11:37:11 -0400 Subject: [PATCH 4/5] fix(settings): adds clientId validation, URL guard, and missing tests --- src/app/components/settings/SettingsPage.tsx | 6 ++- src/app/lib/oauth.ts | 5 ++ .../components/settings/SettingsPage.test.tsx | 52 ++++++++++++++++++- tests/lib/oauth.test.ts | 14 +++++ 4 files changed, 74 insertions(+), 3 deletions(-) diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index 83df33d6..b807792d 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -4,7 +4,7 @@ import { config, updateConfig } from "../../stores/config"; import { clearAuth } from "../../stores/auth"; import { clearCache } from "../../stores/cache"; import { buildOrgAccessUrl } from "../../lib/oauth"; -import { openGitHubUrl } from "../../lib/url"; +import { isSafeGitHubUrl, openGitHubUrl } from "../../lib/url"; import { fetchOrgs } from "../../services/api"; import { getClient } from "../../services/github"; import OrgSelector from "../onboarding/OrgSelector"; @@ -228,7 +228,9 @@ export default function SettingsPage() { } function handleGrantOrgs() { - openGitHubUrl(buildOrgAccessUrl()); + 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); diff --git a/src/app/lib/oauth.ts b/src/app/lib/oauth.ts index e1f901fd..249c6b71 100644 --- a/src/app/lib/oauth.ts +++ b/src/app/lib/oauth.ts @@ -36,7 +36,12 @@ 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}`; } diff --git a/tests/components/settings/SettingsPage.test.tsx b/tests/components/settings/SettingsPage.test.tsx index cf76fafe..91e4b658 100644 --- a/tests/components/settings/SettingsPage.test.tsx +++ b/tests/components/settings/SettingsPage.test.tsx @@ -42,6 +42,7 @@ vi.mock("../../../src/app/services/api", () => ({ })); vi.mock("../../../src/app/lib/url", () => ({ + isSafeGitHubUrl: vi.fn(() => true), openGitHubUrl: vi.fn(), })); @@ -94,6 +95,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, @@ -613,7 +617,7 @@ describe("SettingsPage — Grant more orgs button", () => { const user = userEvent.setup(); vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); const github = await import("../../../src/app/services/github"); - vi.mocked(github.getClient).mockReturnValue(null); + vi.mocked(github.getClient).mockReturnValueOnce(null); renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); await user.click(btn); @@ -623,4 +627,50 @@ describe("SettingsPage — Grant more orgs button", () => { expect(apiModule.fetchOrgs).not.toHaveBeenCalled(); vi.unstubAllEnvs(); }); + + it("rapid double-click deduplicates focus listeners", async () => { + const user = userEvent.setup(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + const removeSpy = vi.spyOn(window, "removeEventListener"); + renderSettings(); + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + await user.click(btn); + // Second click removes the first listener before adding a new one + const focusRemoves = removeSpy.mock.calls.filter(([evt]) => evt === "focus"); + expect(focusRemoves.length).toBeGreaterThanOrEqual(1); + vi.unstubAllEnvs(); + }); + + it("cleans up pending focus listener on component unmount", async () => { + const user = userEvent.setup(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + 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); + vi.unstubAllEnvs(); + }); + + it("focus listener self-removes — second focus does not re-trigger merge", async () => { + const user = userEvent.setup(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + updateConfig({ selectedOrgs: [] }); + vi.mocked(apiModule.fetchOrgs).mockResolvedValue([]); + renderSettings(); + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + window.dispatchEvent(new Event("focus")); + await waitFor(() => { + expect(apiModule.fetchOrgs).toHaveBeenCalledTimes(1); + }); + // Second focus should NOT trigger another merge + window.dispatchEvent(new Event("focus")); + await new Promise((r) => setTimeout(r, 50)); + expect(apiModule.fetchOrgs).toHaveBeenCalledTimes(1); + vi.unstubAllEnvs(); + }); }); diff --git a/tests/lib/oauth.test.ts b/tests/lib/oauth.test.ts index 9d7706c2..222d2506 100644 --- a/tests/lib/oauth.test.ts +++ b/tests/lib/oauth.test.ts @@ -105,6 +105,20 @@ describe("oauth helpers", () => { "https://github.com/settings/connections/applications/test-client-id" ); }); + + it("throws for undefined client ID", () => { + vi.stubEnv("VITE_GITHUB_CLIENT_ID", ""); + expect(() => buildOrgAccessUrl()).toThrow("Invalid VITE_GITHUB_CLIENT_ID"); + vi.unstubAllEnvs(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + }); + + it("throws for client ID with path traversal characters", () => { + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "../../../evil"); + expect(() => buildOrgAccessUrl()).toThrow("Invalid VITE_GITHUB_CLIENT_ID"); + vi.unstubAllEnvs(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + }); }); describe("sanitizeReturnTo", () => { From 70ae341c0d8ddb6594107c724123c25c31c26f79 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 25 Mar 2026 12:31:30 -0400 Subject: [PATCH 5/5] fix(settings): surfaces sync failure notification, tests disabled state --- src/app/components/settings/SettingsPage.tsx | 3 +- .../components/settings/SettingsPage.test.tsx | 57 ++++++++++--------- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index b807792d..3442930b 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -3,6 +3,7 @@ import { useNavigate } from "@solidjs/router"; import { config, updateConfig } from "../../stores/config"; import { clearAuth } from "../../stores/auth"; import { clearCache } from "../../stores/cache"; +import { pushNotification } from "../../lib/errors"; import { buildOrgAccessUrl } from "../../lib/oauth"; import { isSafeGitHubUrl, openGitHubUrl } from "../../lib/url"; import { fetchOrgs } from "../../services/api"; @@ -221,7 +222,7 @@ 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); } diff --git a/tests/components/settings/SettingsPage.test.tsx b/tests/components/settings/SettingsPage.test.tsx index 91e4b658..f5b34409 100644 --- a/tests/components/settings/SettingsPage.test.tsx +++ b/tests/components/settings/SettingsPage.test.tsx @@ -46,6 +46,10 @@ vi.mock("../../../src/app/lib/url", () => ({ openGitHubUrl: vi.fn(), })); +vi.mock("../../../src/app/lib/errors", () => ({ + pushNotification: vi.fn(), +})); + // ── Imports after mocks ─────────────────────────────────────────────────────── import { render } from "@solidjs/testing-library"; @@ -530,6 +534,13 @@ describe("SettingsPage — Theme application", () => { }); 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" }); @@ -537,46 +548,45 @@ describe("SettingsPage — Grant more orgs button", () => { 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(urlModule.openGitHubUrl).toHaveBeenCalledWith(buildOrgAccessUrl()); - vi.unstubAllEnvs(); }); 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(addSpy).toHaveBeenCalledWith("focus", expect.any(Function)); - vi.unstubAllEnvs(); }); - it("shows 'Syncing...' on button while merging and reverts after", async () => { + it("shows disabled 'Syncing...' button during merge, reverts after", async () => { const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); updateConfig({ selectedOrgs: [] }); - vi.mocked(apiModule.fetchOrgs).mockResolvedValue([]); + 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); window.dispatchEvent(new Event("focus")); await waitFor(() => { - expect(screen.getByRole("button", { name: "Syncing..." })).toBeDefined(); + const syncBtn = screen.getByRole("button", { name: "Syncing..." }); + expect(syncBtn.hasAttribute("disabled")).toBe(true); }); + resolveFetch([]); await waitFor(() => { - expect(screen.getByRole("button", { name: "Grant more orgs" })).toBeDefined(); + const restored = screen.getByRole("button", { name: "Grant more orgs" }); + expect(restored.hasAttribute("disabled")).toBe(false); }); - vi.unstubAllEnvs(); }); it("auto-merges new orgs when window regains focus after granting", async () => { const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); updateConfig({ selectedOrgs: ["existing-org"] }); vi.mocked(apiModule.fetchOrgs).mockResolvedValue([ { login: "existing-org", avatarUrl: "", type: "org" }, @@ -585,7 +595,6 @@ describe("SettingsPage — Grant more orgs button", () => { renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); await user.click(btn); - // Simulate user returning from GitHub settings tab window.dispatchEvent(new Event("focus")); await waitFor(() => { expect(apiModule.fetchOrgs).toHaveBeenCalled(); @@ -594,12 +603,11 @@ describe("SettingsPage — Grant more orgs button", () => { expect(config.selectedOrgs).toContain("new-org"); expect(config.selectedOrgs).toContain("existing-org"); }); - vi.unstubAllEnvs(); }); - it("silently handles fetchOrgs rejection on focus without breaking", async () => { + it("pushes warning notification on fetchOrgs failure", async () => { + const { pushNotification } = await import("../../../src/app/lib/errors"); const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); updateConfig({ selectedOrgs: ["existing-org"] }); vi.mocked(apiModule.fetchOrgs).mockRejectedValue(new Error("Network error")); renderSettings(); @@ -609,42 +617,41 @@ describe("SettingsPage — Grant more orgs button", () => { await waitFor(() => { expect(apiModule.fetchOrgs).toHaveBeenCalled(); }); + await waitFor(() => { + expect(pushNotification).toHaveBeenCalledWith( + "org-sync", + expect.stringContaining("Failed to sync"), + "warning", + ); + }); expect(config.selectedOrgs).toEqual(["existing-org"]); - vi.unstubAllEnvs(); }); it("skips merge on focus when getClient returns null", async () => { const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); 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")); - // Give mergeNewOrgs time to bail out await new Promise((r) => setTimeout(r, 50)); expect(apiModule.fetchOrgs).not.toHaveBeenCalled(); - vi.unstubAllEnvs(); }); it("rapid double-click deduplicates focus listeners", async () => { const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); const removeSpy = vi.spyOn(window, "removeEventListener"); renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); await user.click(btn); await user.click(btn); - // Second click removes the first listener before adding a new one const focusRemoves = removeSpy.mock.calls.filter(([evt]) => evt === "focus"); expect(focusRemoves.length).toBeGreaterThanOrEqual(1); - vi.unstubAllEnvs(); }); it("cleans up pending focus listener on component unmount", async () => { const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); const removeSpy = vi.spyOn(window, "removeEventListener"); const { unmount } = renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); @@ -652,12 +659,10 @@ describe("SettingsPage — Grant more orgs button", () => { unmount(); const focusRemoves = removeSpy.mock.calls.filter(([evt]) => evt === "focus"); expect(focusRemoves.length).toBeGreaterThanOrEqual(1); - vi.unstubAllEnvs(); }); it("focus listener self-removes — second focus does not re-trigger merge", async () => { const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); updateConfig({ selectedOrgs: [] }); vi.mocked(apiModule.fetchOrgs).mockResolvedValue([]); renderSettings(); @@ -667,10 +672,8 @@ describe("SettingsPage — Grant more orgs button", () => { await waitFor(() => { expect(apiModule.fetchOrgs).toHaveBeenCalledTimes(1); }); - // Second focus should NOT trigger another merge window.dispatchEvent(new Event("focus")); await new Promise((r) => setTimeout(r, 50)); expect(apiModule.fetchOrgs).toHaveBeenCalledTimes(1); - vi.unstubAllEnvs(); }); });