Skip to content

Commit ed386dc

Browse files
wgordon17wgordon17
committed
fix(settings): opens GitHub app settings for org access
1 parent b2a8713 commit ed386dc

File tree

4 files changed

+72
-93
lines changed

4 files changed

+72
-93
lines changed

‎src/app/components/settings/SettingsPage.tsx‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { useNavigate } from "@solidjs/router";
33
import { config, updateConfig } from "../../stores/config";
44
import { clearAuth } from "../../stores/auth";
55
import { clearCache } from "../../stores/cache";
6-
import { buildAuthorizeUrl, MERGE_ORGS_KEY } from "../../lib/oauth";
6+
import { buildOrgAccessUrl } from "../../lib/oauth";
77
import { fetchOrgs } from "../../services/api";
88
import { getClient } from "../../services/github";
99
import OrgSelector from "../onboarding/OrgSelector";
@@ -153,7 +153,7 @@ export default function SettingsPage() {
153153
const [confirmClearCache, setConfirmClearCache] = createSignal(false);
154154
const [confirmReset, setConfirmReset] = createSignal(false);
155155
const [cacheClearing, setCacheClearing] = createSignal(false);
156-
const [reauthing, setReauthing] = createSignal(false);
156+
const [merging, setMerging] = createSignal(false);
157157
const [notifPermission, setNotifPermission] = createSignal<NotificationPermission>(
158158
typeof Notification !== "undefined" ? Notification.permission : "default"
159159
);
@@ -191,20 +191,14 @@ export default function SettingsPage() {
191191
};
192192
mq.addEventListener("change", handler);
193193
onCleanup(() => mq.removeEventListener("change", handler));
194-
195-
// Auto-merge newly accessible orgs after re-auth redirect
196-
const shouldMerge = sessionStorage.getItem(MERGE_ORGS_KEY);
197-
if (shouldMerge) {
198-
sessionStorage.removeItem(MERGE_ORGS_KEY);
199-
void mergeNewOrgs();
200-
}
201194
});
202195

203196
// ── Helpers ──────────────────────────────────────────────────────────────
204197

205198
async function mergeNewOrgs() {
206199
const client = getClient();
207200
if (!client) return;
201+
setMerging(true);
208202
const snapshot = [...config.selectedOrgs];
209203
try {
210204
const allOrgs = await fetchOrgs(client);
@@ -221,15 +215,20 @@ export default function SettingsPage() {
221215
}
222216
} catch {
223217
// Non-fatal — user can manually manage orgs
218+
} finally {
219+
setMerging(false);
224220
}
225221
}
226222

227-
function handleReAuth() {
228-
setReauthing(true);
229-
// Reset if navigation doesn't happen (e.g., beforeunload dialog blocks redirect)
230-
setTimeout(() => setReauthing(false), 3000);
231-
sessionStorage.setItem(MERGE_ORGS_KEY, "true");
232-
window.location.href = buildAuthorizeUrl({ returnTo: "/settings" });
223+
function handleGrantOrgs() {
224+
window.open(buildOrgAccessUrl(), "_blank", "noopener");
225+
// Auto-merge newly accessible orgs when user returns from GitHub settings
226+
const onFocus = () => {
227+
window.removeEventListener("focus", onFocus);
228+
void mergeNewOrgs();
229+
};
230+
window.addEventListener("focus", onFocus);
231+
onCleanup(() => window.removeEventListener("focus", onFocus));
233232
}
234233

235234
function handleOrgsChange(orgs: string[]) {
@@ -408,16 +407,16 @@ export default function SettingsPage() {
408407
Organization Access
409408
</p>
410409
<p class="text-xs text-gray-500 dark:text-gray-400">
411-
Re-authorize with GitHub to grant access to additional organizations
410+
Manage organization access on GitHub — new orgs sync automatically when you return
412411
</p>
413412
</div>
414413
<button
415414
type="button"
416-
onClick={handleReAuth}
417-
disabled={reauthing()}
415+
onClick={handleGrantOrgs}
416+
disabled={merging()}
418417
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"
419418
>
420-
Grant more orgs
419+
{merging() ? "Syncing..." : "Grant more orgs"}
421420
</button>
422421
</div>
423422
</div>

‎src/app/lib/oauth.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
export const OAUTH_STATE_KEY = "github-tracker:oauth-state";
22
export const OAUTH_RETURN_TO_KEY = "github-tracker:oauth-return-to";
3-
export const MERGE_ORGS_KEY = "github-tracker:merge-orgs";
43

54
export function generateOAuthState(): string {
65
const stateBytes = crypto.getRandomValues(new Uint8Array(16));
@@ -36,3 +35,8 @@ export function buildAuthorizeUrl(options?: { returnTo?: string }): string {
3635
});
3736
return `https://github.com/login/oauth/authorize?${params.toString()}`;
3837
}
38+
39+
export function buildOrgAccessUrl(): string {
40+
const clientId = import.meta.env.VITE_GITHUB_CLIENT_ID as string;
41+
return `https://github.com/settings/connections/applications/${clientId}`;
42+
}

‎tests/components/settings/SettingsPage.test.tsx‎

Lines changed: 39 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ import * as authStore from "../../../src/app/stores/auth";
5050
import * as cacheStore from "../../../src/app/stores/cache";
5151
import * as apiModule from "../../../src/app/services/api";
5252
import { updateConfig, config } from "../../../src/app/stores/config";
53-
import { MERGE_ORGS_KEY, OAUTH_STATE_KEY } from "../../../src/app/lib/oauth";
53+
import { buildOrgAccessUrl } from "../../../src/app/lib/oauth";
5454

5555
// ── Helpers ───────────────────────────────────────────────────────────────────
5656

@@ -104,7 +104,6 @@ beforeEach(() => {
104104
selectedRepos: [],
105105
});
106106

107-
// Clear sessionStorage — re-auth tests set MERGE_ORGS_KEY and OAUTH_STATE_KEY
108107
sessionStorage.clear();
109108

110109
// Mock window.location with both reload and href
@@ -521,126 +520,93 @@ describe("SettingsPage — Theme application", () => {
521520
});
522521
});
523522

524-
describe("SettingsPage — Re-auth button", () => {
523+
describe("SettingsPage — Grant more orgs button", () => {
525524
it("renders 'Grant more orgs' button in Organizations & Repositories section", () => {
526525
renderSettings();
527526
screen.getByRole("button", { name: "Grant more orgs" });
528527
});
529528

530-
it("clicking 'Grant more orgs' sets MERGE_ORGS_KEY in sessionStorage", async () => {
529+
it("clicking 'Grant more orgs' opens GitHub app settings in new tab", async () => {
531530
const user = userEvent.setup();
532531
vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
532+
const openSpy = vi.spyOn(window, "open").mockImplementation(() => null);
533533
renderSettings();
534534
const btn = screen.getByRole("button", { name: "Grant more orgs" });
535535
await user.click(btn);
536-
expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBe("true");
536+
expect(openSpy).toHaveBeenCalledWith(
537+
buildOrgAccessUrl(),
538+
"_blank",
539+
"noopener",
540+
);
537541
vi.unstubAllEnvs();
538542
});
539543

540-
it("clicking 'Grant more orgs' sets window.location.href to GitHub authorize URL", async () => {
544+
it("clicking 'Grant more orgs' registers a focus listener for auto-merge", async () => {
541545
const user = userEvent.setup();
542546
vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
547+
vi.spyOn(window, "open").mockImplementation(() => null);
548+
const addSpy = vi.spyOn(window, "addEventListener");
543549
renderSettings();
544550
const btn = screen.getByRole("button", { name: "Grant more orgs" });
545551
await user.click(btn);
546-
expect(window.location.href).toContain("https://github.com/login/oauth/authorize");
552+
expect(addSpy).toHaveBeenCalledWith("focus", expect.any(Function));
547553
vi.unstubAllEnvs();
548554
});
549555

550-
it("clicking 'Grant more orgs' also sets OAUTH_STATE_KEY in sessionStorage", async () => {
556+
it("auto-merges new orgs when window regains focus after granting", async () => {
551557
const user = userEvent.setup();
552558
vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
553-
renderSettings();
554-
const btn = screen.getByRole("button", { name: "Grant more orgs" });
555-
await user.click(btn);
556-
expect(sessionStorage.getItem(OAUTH_STATE_KEY)).toBeTruthy();
557-
vi.unstubAllEnvs();
558-
});
559-
560-
it("'Grant more orgs' button is disabled after click and re-enables after timeout", async () => {
561-
vi.useFakeTimers();
562-
const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime });
563-
vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
564-
renderSettings();
565-
const btn = screen.getByRole("button", { name: "Grant more orgs" });
566-
await user.click(btn);
567-
expect(btn.hasAttribute("disabled")).toBe(true);
568-
vi.advanceTimersByTime(3000);
569-
expect(btn.hasAttribute("disabled")).toBe(false);
570-
vi.unstubAllEnvs();
571-
vi.useRealTimers();
572-
});
573-
});
574-
575-
describe("SettingsPage — Auto-merge orgs on mount", () => {
576-
it("calls fetchOrgs and merges new orgs when MERGE_ORGS_KEY is in sessionStorage", async () => {
577-
sessionStorage.setItem(MERGE_ORGS_KEY, "true");
559+
vi.spyOn(window, "open").mockImplementation(() => null);
578560
updateConfig({ selectedOrgs: ["existing-org"] });
579561
vi.mocked(apiModule.fetchOrgs).mockResolvedValue([
580562
{ login: "existing-org", avatarUrl: "", type: "org" },
581563
{ login: "new-org", avatarUrl: "", type: "org" },
582564
]);
583565
renderSettings();
566+
const btn = screen.getByRole("button", { name: "Grant more orgs" });
567+
await user.click(btn);
568+
// Simulate user returning from GitHub settings tab
569+
window.dispatchEvent(new Event("focus"));
584570
await waitFor(() => {
585571
expect(apiModule.fetchOrgs).toHaveBeenCalled();
586572
});
587573
await waitFor(() => {
588574
expect(config.selectedOrgs).toContain("new-org");
589-
});
590-
});
591-
592-
it("preserves existing selectedOrgs when merging", async () => {
593-
sessionStorage.setItem(MERGE_ORGS_KEY, "true");
594-
updateConfig({ selectedOrgs: ["existing-org"] });
595-
vi.mocked(apiModule.fetchOrgs).mockResolvedValue([
596-
{ login: "existing-org", avatarUrl: "", type: "org" },
597-
{ login: "new-org", avatarUrl: "", type: "org" },
598-
]);
599-
renderSettings();
600-
await waitFor(() => {
601575
expect(config.selectedOrgs).toContain("existing-org");
602576
});
577+
vi.unstubAllEnvs();
603578
});
604579

605-
it("removes MERGE_ORGS_KEY from sessionStorage after processing", async () => {
606-
sessionStorage.setItem(MERGE_ORGS_KEY, "true");
607-
updateConfig({ selectedOrgs: [] });
608-
vi.mocked(apiModule.fetchOrgs).mockResolvedValue([
609-
{ login: "new-org", avatarUrl: "", type: "org" },
610-
]);
611-
renderSettings();
612-
await waitFor(() => {
613-
expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBeNull();
614-
});
615-
});
616-
617-
it("does not call fetchOrgs when MERGE_ORGS_KEY is not in sessionStorage", () => {
618-
// No MERGE_ORGS_KEY set
619-
renderSettings();
620-
expect(apiModule.fetchOrgs).not.toHaveBeenCalled();
621-
});
622-
623-
it("silently handles fetchOrgs rejection without breaking", async () => {
624-
sessionStorage.setItem(MERGE_ORGS_KEY, "true");
580+
it("silently handles fetchOrgs rejection on focus without breaking", async () => {
581+
const user = userEvent.setup();
582+
vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
583+
vi.spyOn(window, "open").mockImplementation(() => null);
625584
updateConfig({ selectedOrgs: ["existing-org"] });
626585
vi.mocked(apiModule.fetchOrgs).mockRejectedValue(new Error("Network error"));
627586
renderSettings();
587+
const btn = screen.getByRole("button", { name: "Grant more orgs" });
588+
await user.click(btn);
589+
window.dispatchEvent(new Event("focus"));
628590
await waitFor(() => {
629591
expect(apiModule.fetchOrgs).toHaveBeenCalled();
630592
});
631-
// Config unchanged — error was swallowed
632593
expect(config.selectedOrgs).toEqual(["existing-org"]);
594+
vi.unstubAllEnvs();
633595
});
634596

635-
it("skips merge when getClient returns null", async () => {
636-
sessionStorage.setItem(MERGE_ORGS_KEY, "true");
597+
it("skips merge on focus when getClient returns null", async () => {
598+
const user = userEvent.setup();
599+
vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id");
600+
vi.spyOn(window, "open").mockImplementation(() => null);
637601
const github = await import("../../../src/app/services/github");
638-
vi.mocked(github.getClient).mockReturnValueOnce(null);
602+
vi.mocked(github.getClient).mockReturnValue(null);
639603
renderSettings();
640-
// MERGE_ORGS_KEY still removed synchronously before mergeNewOrgs
641-
await waitFor(() => {
642-
expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBeNull();
643-
});
604+
const btn = screen.getByRole("button", { name: "Grant more orgs" });
605+
await user.click(btn);
606+
window.dispatchEvent(new Event("focus"));
607+
// Give mergeNewOrgs time to bail out
608+
await new Promise((r) => setTimeout(r, 50));
644609
expect(apiModule.fetchOrgs).not.toHaveBeenCalled();
610+
vi.unstubAllEnvs();
645611
});
646612
});

‎tests/lib/oauth.test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
22
import {
33
generateOAuthState,
44
buildAuthorizeUrl,
5+
buildOrgAccessUrl,
56
sanitizeReturnTo,
67
OAUTH_STATE_KEY,
78
OAUTH_RETURN_TO_KEY,
@@ -97,6 +98,15 @@ describe("oauth helpers", () => {
9798
});
9899
});
99100

101+
describe("buildOrgAccessUrl", () => {
102+
it("returns GitHub connections URL with client ID", () => {
103+
const url = buildOrgAccessUrl();
104+
expect(url).toBe(
105+
"https://github.com/settings/connections/applications/test-client-id"
106+
);
107+
});
108+
});
109+
100110
describe("sanitizeReturnTo", () => {
101111
it("accepts internal paths", () => {
102112
expect(sanitizeReturnTo("/settings")).toBe("/settings");

0 commit comments

Comments
 (0)