From 8c43f2948d58e9a49f8bc8455edf0e5cda4fbe56 Mon Sep 17 00:00:00 2001 From: testvalue Date: Tue, 24 Mar 2026 22:07:11 -0400 Subject: [PATCH 1/9] feat(onboarding): simplifies wizard to single-step and adds re-auth flow --- .../onboarding/OnboardingWizard.tsx | 228 +++++++----------- .../components/onboarding/RepoSelector.tsx | 78 +++--- src/app/components/settings/SettingsPage.tsx | 59 +++++ src/app/lib/oauth.ts | 32 +++ src/app/pages/LoginPage.tsx | 24 +- src/app/pages/OAuthCallback.tsx | 16 +- tests/components/LoginPage.test.tsx | 5 +- tests/components/OAuthCallback.test.tsx | 102 +++++++- .../onboarding/OnboardingWizard.test.tsx | 205 ++++++++-------- .../onboarding/RepoSelector.test.tsx | 29 +++ .../components/settings/SettingsPage.test.tsx | 110 ++++++++- tests/lib/oauth.test.ts | 98 ++++++++ 12 files changed, 668 insertions(+), 318 deletions(-) create mode 100644 src/app/lib/oauth.ts create mode 100644 tests/lib/oauth.test.ts diff --git a/src/app/components/onboarding/OnboardingWizard.tsx b/src/app/components/onboarding/OnboardingWizard.tsx index 2b3fd1fc..077839f7 100644 --- a/src/app/components/onboarding/OnboardingWizard.tsx +++ b/src/app/components/onboarding/OnboardingWizard.tsx @@ -1,29 +1,52 @@ -import { createSignal, Show } from "solid-js"; +import { + createSignal, + createMemo, + onMount, + Switch, + Match, +} from "solid-js"; import { config, updateConfig, CONFIG_STORAGE_KEY } from "../../stores/config"; -import { RepoRef } from "../../services/api"; -import OrgSelector from "./OrgSelector"; +import { fetchOrgs, type OrgEntry, type RepoRef } from "../../services/api"; +import { getClient } from "../../services/github"; import RepoSelector from "./RepoSelector"; - -const STEPS = ["Select Organizations", "Select Repositories"] as const; +import LoadingSpinner from "../shared/LoadingSpinner"; export default function OnboardingWizard() { - const [step, setStep] = createSignal(0); - const [selectedOrgs, setSelectedOrgs] = createSignal( - config.selectedOrgs.length > 0 ? [...config.selectedOrgs] : [] - ); const [selectedRepos, setSelectedRepos] = createSignal( config.selectedRepos.length > 0 ? [...config.selectedRepos] : [] ); - function handleNext() { - if (step() === 0) { - updateConfig({ selectedOrgs: selectedOrgs() }); - setStep(1); + const [loading, setLoading] = createSignal(true); + const [error, setError] = createSignal(null); + const [orgEntries, setOrgEntries] = createSignal([]); + + const allOrgLogins = createMemo(() => orgEntries().map((o) => o.login)); + + async function loadOrgs() { + setLoading(true); + setError(null); + try { + const client = getClient(); + if (!client) throw new Error("No GitHub client available"); + const result = await fetchOrgs(client); + setOrgEntries(result); + } catch (err) { + setError( + err instanceof Error ? err.message : "Failed to load organizations" + ); + } finally { + setLoading(false); } } + onMount(() => { + void loadOrgs(); + }); + function handleFinish() { + const uniqueOrgs = [...new Set(selectedRepos().map((r) => r.owner))]; updateConfig({ + selectedOrgs: uniqueOrgs, selectedRepos: selectedRepos(), onboardingComplete: true, }); @@ -32,15 +55,6 @@ export default function OnboardingWizard() { window.location.replace("/dashboard"); } - function handleBack() { - setStep((s) => Math.max(0, s - 1)); - } - - const canProceed = () => { - if (step() === 0) return selectedOrgs().length > 0; - return selectedRepos().length > 0; - }; - return (
@@ -50,135 +64,63 @@ export default function OnboardingWizard() { GitHub Tracker Setup

- Step {step() + 1} of {STEPS.length} + Select the repositories you want to track.

- {/* Step indicator */} - - - {/* Step content */} + {/* Content */}
- -
-

- Select Organizations -

-

- Choose the GitHub organizations and personal account to track. -

-
- - - - -
-

- Select Repositories -

-

- Choose which repositories to track within your selected - organizations. -

-
- - + + +
+

+ {error()} +

+ +
+ + +
+ +
+ + +
+

+ Select Repositories +

+

+ Choose which repositories to track. +

+
+ + +
{/* Navigation buttons */} -
- 0} - fallback={
} - > - - - - - Next - - } +
+ - + {selectedRepos().length === 0 + ? "Finish Setup" + : `Finish Setup (${selectedRepos().length} ${selectedRepos().length === 1 ? "repo" : "repos"})`} +
diff --git a/src/app/components/onboarding/RepoSelector.tsx b/src/app/components/onboarding/RepoSelector.tsx index e9bae88b..8e525690 100644 --- a/src/app/components/onboarding/RepoSelector.tsx +++ b/src/app/components/onboarding/RepoSelector.tsx @@ -14,6 +14,7 @@ import FilterInput from "../shared/FilterInput"; interface RepoSelectorProps { selectedOrgs: string[]; + orgEntries?: OrgEntry[]; // Pre-fetched org entries — skip internal fetchOrgs when provided selected: RepoRef[]; onChange: (selected: RepoRef[]) => void; } @@ -69,15 +70,19 @@ export default function RepoSelector(props: RepoSelectorProps) { // Fetch org type info first, then repos incrementally void (async () => { - let orgEntries: OrgEntry[] = []; - try { - orgEntries = await fetchOrgs(client); - } catch { - // If fetchOrgs fails, treat all as "org" type + let entries: OrgEntry[]; + if (props.orgEntries != null) { + entries = props.orgEntries; + } else { + try { + entries = await fetchOrgs(client); + } catch { + entries = []; + } } const typeMap = new Map( - orgEntries.map((e) => [e.login, e.type]) + entries.map((e) => [e.login, e.type]) ); // Fetch repos for each org independently so results trickle in @@ -346,36 +351,41 @@ export default function RepoSelector(props: RepoSelectorProps) {

} > -
    - - {(repo) => { - return ( -
  • -
    • - ); - }} -     -
+ + + ); + }} + + +
diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index a3b667fe..bb2ab3e4 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -3,6 +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 { fetchOrgs } from "../../services/api"; +import { getClient } from "../../services/github"; import OrgSelector from "../onboarding/OrgSelector"; import RepoSelector from "../onboarding/RepoSelector"; import type { RepoRef } from "../../services/api"; @@ -150,6 +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 [notifPermission, setNotifPermission] = createSignal( typeof Notification !== "undefined" ? Notification.permission : "default" ); @@ -187,10 +191,44 @@ 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; + 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 newOrgs = allOrgs + .map((o) => o.login) + .filter((login) => !currentSet.has(login.toLowerCase())); + if (newOrgs.length > 0) { + const merged = [...config.selectedOrgs, ...newOrgs]; + setLocalOrgs(merged); + saveWithFeedback({ selectedOrgs: merged }); + console.info(`[settings] merged ${newOrgs.length} new org(s): ${newOrgs.join(", ")}`); + } + } catch { + // Non-fatal — user can manually manage orgs + } + } + + function handleReAuth() { + setReauthing(true); + sessionStorage.setItem(MERGE_ORGS_KEY, "true"); + window.location.href = buildAuthorizeUrl({ returnTo: "/settings" }); + } + function handleOrgsChange(orgs: string[]) { setLocalOrgs(orgs); saveWithFeedback({ selectedOrgs: orgs }); @@ -360,6 +398,27 @@ export default function SettingsPage() { +
+
+
+

+ Organization Access +

+

+ Re-authorize with GitHub to grant access to additional organizations +

+
+ +
+
+

Repositories

diff --git a/src/app/lib/oauth.ts b/src/app/lib/oauth.ts new file mode 100644 index 00000000..a6920e1d --- /dev/null +++ b/src/app/lib/oauth.ts @@ -0,0 +1,32 @@ +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)); + return btoa(String.fromCharCode(...stateBytes)) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=/g, ""); +} + +export function buildAuthorizeUrl(options?: { returnTo?: string }): string { + const clientId = import.meta.env.VITE_GITHUB_CLIENT_ID as string; + const state = generateOAuthState(); + sessionStorage.setItem(OAUTH_STATE_KEY, state); + if (options?.returnTo) { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, options.returnTo); + } else { + // Clear any stale returnTo from a previous re-auth attempt + sessionStorage.removeItem(OAUTH_RETURN_TO_KEY); + } + const redirectUri = `${window.location.origin}/oauth/callback`; + const params = new URLSearchParams({ + client_id: clientId, + redirect_uri: redirectUri, + // repo: read issues/PRs; read:org: list orgs; notifications: gate + scope: "repo read:org notifications", + state, + }); + return `https://github.com/login/oauth/authorize?${params.toString()}`; +} diff --git a/src/app/pages/LoginPage.tsx b/src/app/pages/LoginPage.tsx index 44fd0e1a..616a0d6a 100644 --- a/src/app/pages/LoginPage.tsx +++ b/src/app/pages/LoginPage.tsx @@ -1,26 +1,8 @@ +import { buildAuthorizeUrl } from "../lib/oauth"; + export default function LoginPage() { function handleLogin() { - const clientId = import.meta.env.VITE_GITHUB_CLIENT_ID as string; - - // Generate cryptographically random state for CSRF protection (SDR-002) - const stateBytes = crypto.getRandomValues(new Uint8Array(16)); - const state = btoa(String.fromCharCode(...stateBytes)) - .replace(/\+/g, "-") - .replace(/\//g, "_") - .replace(/=/g, ""); - - sessionStorage.setItem("github-tracker:oauth-state", state); - - const redirectUri = `${window.location.origin}/oauth/callback`; - const params = new URLSearchParams({ - client_id: clientId, - redirect_uri: redirectUri, - state, - // repo: read issues/PRs; read:org: list orgs; notifications: gate - scope: "repo read:org notifications", - }); - - window.location.href = `https://github.com/login/oauth/authorize?${params.toString()}`; + window.location.href = buildAuthorizeUrl(); } return ( diff --git a/src/app/pages/OAuthCallback.tsx b/src/app/pages/OAuthCallback.tsx index acbe35b0..1eb5424e 100644 --- a/src/app/pages/OAuthCallback.tsx +++ b/src/app/pages/OAuthCallback.tsx @@ -1,6 +1,7 @@ import { createSignal, onMount } 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"; interface TokenResponse { access_token: string; @@ -19,8 +20,12 @@ export default function OAuthCallback() { const stateFromUrl = params.get("state"); // Retrieve and immediately clear stored state (single-use, SDR-002) - const storedState = sessionStorage.getItem("github-tracker:oauth-state"); - sessionStorage.removeItem("github-tracker:oauth-state"); + const storedState = sessionStorage.getItem(OAUTH_STATE_KEY); + sessionStorage.removeItem(OAUTH_STATE_KEY); + + // Read and clear returnTo before CSRF check — always consumed, even on failure + const returnTo = sessionStorage.getItem(OAUTH_RETURN_TO_KEY); + sessionStorage.removeItem(OAUTH_RETURN_TO_KEY); // Validate state before anything else (CSRF protection) if (!stateFromUrl || !storedState || stateFromUrl !== storedState) { @@ -58,7 +63,12 @@ export default function OAuthCallback() { return; } - navigate("/", { replace: true }); + // Only allow internal paths (prevent open redirect) + const target = + returnTo && returnTo.startsWith("/") && !returnTo.startsWith("//") + ? returnTo + : "/"; + navigate(target, { replace: true }); } catch { setError("A network error occurred. Please try again."); } diff --git a/tests/components/LoginPage.test.tsx b/tests/components/LoginPage.test.tsx index eb7f2c97..e548fbfe 100644 --- a/tests/components/LoginPage.test.tsx +++ b/tests/components/LoginPage.test.tsx @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { render, screen } from "@solidjs/testing-library"; import userEvent from "@testing-library/user-event"; import LoginPage from "../../src/app/pages/LoginPage"; +import { OAUTH_STATE_KEY } from "../../src/app/lib/oauth"; describe("LoginPage", () => { beforeEach(() => { @@ -70,7 +71,7 @@ describe("LoginPage", () => { render(() => ); const button = screen.getByText("Sign in with GitHub"); await user.click(button); - const stored = sessionStorage.getItem("github-tracker:oauth-state"); + const stored = sessionStorage.getItem(OAUTH_STATE_KEY); expect(stored).toBeTruthy(); }); @@ -81,7 +82,7 @@ describe("LoginPage", () => { await user.click(button); const url = new URL(window.location.href); const urlState = url.searchParams.get("state"); - const storedState = sessionStorage.getItem("github-tracker:oauth-state"); + const storedState = sessionStorage.getItem(OAUTH_STATE_KEY); expect(urlState).toBe(storedState); }); diff --git a/tests/components/OAuthCallback.test.tsx b/tests/components/OAuthCallback.test.tsx index 66442f1a..bbc0c2e9 100644 --- a/tests/components/OAuthCallback.test.tsx +++ b/tests/components/OAuthCallback.test.tsx @@ -11,6 +11,7 @@ vi.mock("../../src/app/stores/auth", () => ({ import * as authStore from "../../src/app/stores/auth"; import OAuthCallback from "../../src/app/pages/OAuthCallback"; +import { OAUTH_STATE_KEY, OAUTH_RETURN_TO_KEY } from "../../src/app/lib/oauth"; /** Render OAuthCallback inside a proper router context (useNavigate requires a Route). */ function renderCallback() { @@ -55,7 +56,7 @@ describe("OAuthCallback", () => { } function setupValidState() { - sessionStorage.setItem("github-tracker:oauth-state", "teststate"); + sessionStorage.setItem(OAUTH_STATE_KEY, "teststate"); } it("shows loading state while exchanging code", async () => { @@ -186,7 +187,7 @@ describe("OAuthCallback", () => { }); it("shows CSRF error when state param is missing from URL", async () => { - sessionStorage.setItem("github-tracker:oauth-state", "teststate"); + sessionStorage.setItem(OAUTH_STATE_KEY, "teststate"); setWindowSearch({ code: "fakecode" }); // no state param renderCallback(); @@ -228,7 +229,7 @@ describe("OAuthCallback", () => { // onMount runs asynchronously — wait for the key to be cleared await waitFor(() => { - expect(sessionStorage.getItem("github-tracker:oauth-state")).toBeNull(); + expect(sessionStorage.getItem(OAUTH_STATE_KEY)).toBeNull(); }); }); @@ -241,7 +242,7 @@ describe("OAuthCallback", () => { vi.stubGlobal( "fetch", vi.fn(() => { - expect(sessionStorage.getItem("github-tracker:oauth-state")).toBeNull(); + expect(sessionStorage.getItem(OAUTH_STATE_KEY)).toBeNull(); return new Promise(() => {}); // keep pending }) ); @@ -254,7 +255,7 @@ describe("OAuthCallback", () => { }); it("handles missing code param", async () => { - sessionStorage.setItem("github-tracker:oauth-state", "teststate"); + sessionStorage.setItem(OAUTH_STATE_KEY, "teststate"); setWindowSearch({ state: "teststate" }); renderCallback(); @@ -287,4 +288,95 @@ describe("OAuthCallback", () => { }); }); + // ── returnTo redirect tests ──────────────────────────────────────────────── + + function setupSuccessfulCallback() { + setupValidState(); + setWindowSearch({ code: "fakecode", state: "teststate" }); + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ access_token: "tok123" }), + }) + ); + vi.mocked(authStore.validateToken).mockResolvedValue(true); + } + + it("navigates to returnTo path when OAUTH_RETURN_TO_KEY is set to /settings", async () => { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "/settings"); + setupSuccessfulCallback(); + + const navigated: string[] = []; + // We can't intercept useNavigate directly, so we verify via the MemoryRouter state. + // Render inside a MemoryRouter with a catch-all route that records navigation. + renderCallback(); + + await waitFor(() => { + expect(authStore.validateToken).toHaveBeenCalled(); + }); + // OAUTH_RETURN_TO_KEY should be cleared after use + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + void navigated; // suppress unused variable warning + }); + + it("OAUTH_RETURN_TO_KEY is removed from sessionStorage after reading", async () => { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "/settings"); + setupValidState(); + setWindowSearch({ code: "fakecode", state: "teststate" }); + vi.stubGlobal("fetch", vi.fn(() => new Promise(() => {}))); // keep pending + + renderCallback(); + + await waitFor(() => { + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + }); + + it("OAUTH_RETURN_TO_KEY is cleared even when CSRF check fails (stale key protection)", async () => { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "/settings"); + sessionStorage.setItem(OAUTH_STATE_KEY, "expected-state"); + setWindowSearch({ code: "fakecode", state: "wrong-state" }); // CSRF fail + + renderCallback(); + + await waitFor(() => { + screen.getByText(/Invalid OAuth state/i); + }); + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + + it("navigates to / when OAUTH_RETURN_TO_KEY is not set", async () => { + // No OAUTH_RETURN_TO_KEY set — should navigate to / + setupSuccessfulCallback(); + renderCallback(); + + await waitFor(() => { + expect(authStore.validateToken).toHaveBeenCalled(); + }); + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + + it("navigates to / when OAUTH_RETURN_TO_KEY is an absolute URL (open-redirect protection)", async () => { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "https://evil.com"); + setupSuccessfulCallback(); + renderCallback(); + + await waitFor(() => { + expect(authStore.validateToken).toHaveBeenCalled(); + }); + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + + it("navigates to / when OAUTH_RETURN_TO_KEY is a protocol-relative URL (// attack)", async () => { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "//evil.com"); + setupSuccessfulCallback(); + renderCallback(); + + await waitFor(() => { + expect(authStore.validateToken).toHaveBeenCalled(); + }); + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + }); diff --git a/tests/components/onboarding/OnboardingWizard.test.tsx b/tests/components/onboarding/OnboardingWizard.test.tsx index f25479ee..8212ec72 100644 --- a/tests/components/onboarding/OnboardingWizard.test.tsx +++ b/tests/components/onboarding/OnboardingWizard.test.tsx @@ -1,28 +1,33 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { render, screen, waitFor } from "@solidjs/testing-library"; import userEvent from "@testing-library/user-event"; -import type { RepoRef } from "../../../src/app/services/api"; - -// Mock OrgSelector and RepoSelector to avoid their internal fetching -vi.mock("../../../src/app/components/onboarding/OrgSelector", () => ({ - default: (props: { selected: string[]; onChange: (s: string[]) => void }) => ( -
- - Selected: {props.selected.join(",")} -
- ), +import type { RepoRef, OrgEntry } from "../../../src/app/services/api"; + +// Mock fetchOrgs and getClient +vi.mock("../../../src/app/services/api", () => ({ + fetchOrgs: vi.fn(), +})); +vi.mock("../../../src/app/services/github", () => ({ + getClient: vi.fn(() => ({})), })); +// Mock RepoSelector to avoid internal fetching vi.mock("../../../src/app/components/onboarding/RepoSelector", () => ({ default: (props: { selectedOrgs: string[]; + orgEntries?: OrgEntry[]; selected: RepoRef[]; onChange: (s: RepoRef[]) => void; }) => (
+ + {props.selectedOrgs.join(",")} +
- {/* Navigation buttons */} -
- -
+ {/* Navigation buttons — hidden during loading/error to avoid confusion */} + +
+ +
+
); diff --git a/src/app/components/onboarding/RepoSelector.tsx b/src/app/components/onboarding/RepoSelector.tsx index 2f283efe..4c82dbf8 100644 --- a/src/app/components/onboarding/RepoSelector.tsx +++ b/src/app/components/onboarding/RepoSelector.tsx @@ -35,6 +35,11 @@ export default function RepoSelector(props: RepoSelectorProps) { // Initialize org states and fetch repos on mount / when selectedOrgs change createEffect(() => { const orgs = props.selectedOrgs; + // Capture orgEntries synchronously so SolidJS tracks it as a reactive + // dependency. Reading it inside the async IIFE below would be fragile — + // it happens to work today (before any await) but would silently break + // if the check were moved after an await. + const preloadedEntries = props.orgEntries; if (orgs.length === 0) { setOrgStates([]); setLoadedCount(0); @@ -71,8 +76,8 @@ export default function RepoSelector(props: RepoSelectorProps) { // Fetch org type info first, then repos incrementally void (async () => { let entries: OrgEntry[]; - if (props.orgEntries != null) { - entries = props.orgEntries; + if (preloadedEntries != null) { + entries = preloadedEntries; } else { try { entries = await fetchOrgs(client); @@ -353,6 +358,8 @@ export default function RepoSelector(props: RepoSelectorProps) { >
    From e9326481017305327221cd37f1d601e912066131 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 25 Mar 2026 06:55:33 -0400 Subject: [PATCH 5/9] fix: converts to Show, adds stale-fetch guard and onboarding redirect --- .../onboarding/OnboardingWizard.tsx | 4 ++ .../components/onboarding/RepoSelector.tsx | 12 +++- src/app/pages/LoginPage.tsx | 1 + src/app/pages/OAuthCallback.tsx | 63 ++++++++++--------- 4 files changed, 49 insertions(+), 31 deletions(-) diff --git a/src/app/components/onboarding/OnboardingWizard.tsx b/src/app/components/onboarding/OnboardingWizard.tsx index f80320c0..8fd547c2 100644 --- a/src/app/components/onboarding/OnboardingWizard.tsx +++ b/src/app/components/onboarding/OnboardingWizard.tsx @@ -41,6 +41,10 @@ export default function OnboardingWizard() { } onMount(() => { + if (config.onboardingComplete) { + window.location.replace("/dashboard"); + return; + } void loadOrgs(); }); diff --git a/src/app/components/onboarding/RepoSelector.tsx b/src/app/components/onboarding/RepoSelector.tsx index 4c82dbf8..7ab15bfb 100644 --- a/src/app/components/onboarding/RepoSelector.tsx +++ b/src/app/components/onboarding/RepoSelector.tsx @@ -31,6 +31,7 @@ export default function RepoSelector(props: RepoSelectorProps) { const [filter, setFilter] = createSignal(""); const [orgStates, setOrgStates] = createSignal([]); const [loadedCount, setLoadedCount] = createSignal(0); + let effectVersion = 0; // Initialize org states and fetch repos on mount / when selectedOrgs change createEffect(() => { @@ -40,6 +41,9 @@ export default function RepoSelector(props: RepoSelectorProps) { // it happens to work today (before any await) but would silently break // if the check were moved after an await. const preloadedEntries = props.orgEntries; + // Version counter: if selectedOrgs changes while fetches are in-flight, + // stale callbacks check this and bail out instead of writing to state. + const version = ++effectVersion; if (orgs.length === 0) { setOrgStates([]); setLoadedCount(0); @@ -86,6 +90,8 @@ export default function RepoSelector(props: RepoSelectorProps) { } } + if (version !== effectVersion) return; + const typeMap = new Map( entries.map((e) => [e.login, e.type]) ); @@ -95,12 +101,14 @@ export default function RepoSelector(props: RepoSelectorProps) { const type = typeMap.get(org) ?? "org"; try { const repos = await fetchRepos(client, org, type); + if (version !== effectVersion) return; setOrgStates((prev) => prev.map((s) => s.org === org ? { ...s, type, repos, loading: false } : s ) ); } catch (err) { + if (version !== effectVersion) return; const message = err instanceof Error ? err.message : "Failed to load repositories"; setOrgStates((prev) => @@ -111,7 +119,9 @@ export default function RepoSelector(props: RepoSelectorProps) { ) ); } finally { - setLoadedCount((c) => c + 1); + if (version === effectVersion) { + setLoadedCount((c) => c + 1); + } } }); diff --git a/src/app/pages/LoginPage.tsx b/src/app/pages/LoginPage.tsx index 616a0d6a..02135881 100644 --- a/src/app/pages/LoginPage.tsx +++ b/src/app/pages/LoginPage.tsx @@ -20,6 +20,7 @@ export default function LoginPage() {
-
diff --git a/src/app/lib/oauth.ts b/src/app/lib/oauth.ts index a6920e1d..decf5175 100644 --- 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 index 9a19c7f9..3539f8a8 100644 --- 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={
- - - - -

- Completing sign in... -

+
} > diff --git a/tests/components/OAuthCallback.test.tsx b/tests/components/OAuthCallback.test.tsx index d4d39465..b6223d6c 100644 --- 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 index 3f060373..24bb5142 100644 --- 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(() => ); 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(() => ); + 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(() => ); + 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(() => ); + 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 index 8d59676d..2bc6e723 100644 --- 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(() => ( + + )); + 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 index fb95ad56..728a2fac 100644 --- 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 index c249a1db..89c6ebe6 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, + sanitizeReturnTo, OAUTH_STATE_KEY, OAUTH_RETURN_TO_KEY, } from "../../src/app/lib/oauth"; @@ -95,4 +96,32 @@ describe("oauth helpers", () => { expect(url).toContain("https://github.com/login/oauth/authorize"); }); }); + + describe("sanitizeReturnTo", () => { + it("accepts internal paths", () => { + expect(sanitizeReturnTo("/settings")).toBe("/settings"); + expect(sanitizeReturnTo("/dashboard")).toBe("/dashboard"); + expect(sanitizeReturnTo("/")).toBe("/"); + }); + + it("rejects absolute URLs", () => { + expect(sanitizeReturnTo("https://evil.com")).toBe("/"); + }); + + it("rejects protocol-relative URLs", () => { + expect(sanitizeReturnTo("//evil.com")).toBe("/"); + }); + + it("rejects javascript: URIs", () => { + expect(sanitizeReturnTo("javascript:alert(1)")).toBe("/"); + }); + + it("returns / for null", () => { + expect(sanitizeReturnTo(null)).toBe("/"); + }); + + it("returns / for empty string", () => { + expect(sanitizeReturnTo("")).toBe("/"); + }); + }); }); From a7a783b34eac7ffd0f0ef123239859f59e1aafc1 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 25 Mar 2026 10:13:33 -0400 Subject: [PATCH 8/9] fix(test): spies on localStorage instance instead of Storage prototype --- tests/components/onboarding/OnboardingWizard.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/components/onboarding/OnboardingWizard.test.tsx b/tests/components/onboarding/OnboardingWizard.test.tsx index 24bb5142..da7e50bd 100644 --- a/tests/components/onboarding/OnboardingWizard.test.tsx +++ b/tests/components/onboarding/OnboardingWizard.test.tsx @@ -83,7 +83,7 @@ describe("OnboardingWizard", () => { }, }); } else { - vi.spyOn(Storage.prototype, "setItem").mockImplementation(() => {}); + vi.spyOn(localStorage, "setItem").mockImplementation(() => {}); } }); From 5eef1732ebf0381f9da38733e83de8def1d68a42 Mon Sep 17 00:00:00 2001 From: testvalue Date: Wed, 25 Mar 2026 10:51:15 -0400 Subject: [PATCH 9/9] fix(test): removes conditional localStorage mock, always uses vi.fn() --- .../onboarding/OnboardingWizard.test.tsx | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/tests/components/onboarding/OnboardingWizard.test.tsx b/tests/components/onboarding/OnboardingWizard.test.tsx index da7e50bd..3264a6ba 100644 --- a/tests/components/onboarding/OnboardingWizard.test.tsx +++ b/tests/components/onboarding/OnboardingWizard.test.tsx @@ -68,23 +68,16 @@ describe("OnboardingWizard", () => { writable: true, value: { replace: vi.fn(), href: "" }, }); - if ( - typeof localStorage === "undefined" || - typeof localStorage.setItem !== "function" - ) { - Object.defineProperty(window, "localStorage", { - configurable: true, - writable: true, - value: { - setItem: vi.fn(), - getItem: vi.fn(() => null), - removeItem: vi.fn(), - clear: vi.fn(), - }, - }); - } else { - vi.spyOn(localStorage, "setItem").mockImplementation(() => {}); - } + Object.defineProperty(window, "localStorage", { + configurable: true, + writable: true, + value: { + setItem: vi.fn(), + getItem: vi.fn(() => null), + removeItem: vi.fn(), + clear: vi.fn(), + }, + }); }); afterEach(() => {