fix: addresses PR review findings from code review

wgordon17 · wgordon17 · commit 1086d16f22ae · 2026-03-31T10:35:00.000-04:00
diff --git a/src/app/components/onboarding/OnboardingWizard.tsx b/src/app/components/onboarding/OnboardingWizard.tsx
@@ -57,14 +57,11 @@ export default function OnboardingWizard() {
 
   function handleFinish() {
     const uniqueOrgs = [...new Set(selectedRepos().map((r) => r.owner))];
-    // Prune monitoredRepos to only repos still in selectedRepos
-    const selectedSet = new Set(selectedRepos().map((r) => r.fullName));
-    const prunedMonitoredRepos = monitoredRepos().filter((r) => selectedSet.has(r.fullName));
     updateConfig({
       selectedOrgs: uniqueOrgs,
       selectedRepos: selectedRepos(),
       upstreamRepos: upstreamRepos(),
-      monitoredRepos: prunedMonitoredRepos,
+      monitoredRepos: monitoredRepos(),
       onboardingComplete: true,
     });
     // Flush synchronously — the debounced persistence effect won't fire before page unload
diff --git a/src/app/services/api.ts b/src/app/services/api.ts
@@ -1058,6 +1058,34 @@ async function executeLightCombinedQuery(
   }
 }
 
+/** Cap-check, notify, and assemble a LightSearchResult from working collections. */
+function finalizeSearchResult(
+  issues: Issue[],
+  prMap: Map<number, PullRequest>,
+  nodeIdMap: Map<number, string>,
+  errors: ApiError[],
+  issueSource: string,
+  prSource: string,
+  issueLabel: string,
+  prLabel: string,
+): LightSearchResult {
+  if (issues.length >= SEARCH_RESULT_CAP) {
+    console.warn(`[api] ${issueLabel} capped at ${SEARCH_RESULT_CAP}`);
+    pushNotification(issueSource, `${issueLabel} capped at ${SEARCH_RESULT_CAP.toLocaleString("en-US")} — some items are hidden`, "warning");
+    issues.splice(SEARCH_RESULT_CAP);
+  }
+
+  if (prMap.size >= SEARCH_RESULT_CAP) {
+    console.warn(`[api] ${prLabel} capped at ${SEARCH_RESULT_CAP}`);
+    pushNotification(prSource, `${prLabel} capped at ${SEARCH_RESULT_CAP.toLocaleString("en-US")} — some items are hidden`, "warning");
+  }
+
+  const pullRequests = [...prMap.values()];
+  if (pullRequests.length >= SEARCH_RESULT_CAP) pullRequests.splice(SEARCH_RESULT_CAP);
+  const prNodeIds = pullRequests.map((pr) => nodeIdMap.get(pr.id)).filter((id): id is string => id != null);
+  return { issues, pullRequests, prNodeIds, errors };
+}
+
 /**
  * Phase 1: light combined search. Fetches issues fully and PRs with minimal fields.
  * Returns light PRs (enriched: false) and their GraphQL node IDs for phase 2 backfill.
@@ -1101,22 +1129,11 @@ async function graphqlLightCombinedSearch(
     }
   }));
 
-  if (issues.length >= SEARCH_RESULT_CAP) {
-    console.warn(`[api] Issue search results capped at ${SEARCH_RESULT_CAP}`);
-    pushNotification("search/issues", `Issue search results capped at ${SEARCH_RESULT_CAP.toLocaleString("en-US")} — some items are hidden`, "warning");
-    issues.splice(SEARCH_RESULT_CAP);
-  }
-
-  if (prMap.size >= SEARCH_RESULT_CAP) {
-    console.warn(`[api] PR search results capped at ${SEARCH_RESULT_CAP}`);
-    pushNotification("search/prs", `PR search results capped at ${SEARCH_RESULT_CAP.toLocaleString("en-US")} — some items are hidden`, "warning");
-  }
-
-  const pullRequests = [...prMap.values()];
-  if (pullRequests.length >= SEARCH_RESULT_CAP) pullRequests.splice(SEARCH_RESULT_CAP);
-  const prNodeIds = pullRequests.map((pr) => nodeIdMap.get(pr.id)).filter((id): id is string => id != null);
-
-  return { issues, pullRequests, prNodeIds, errors };
+  return finalizeSearchResult(
+    issues, prMap, nodeIdMap, errors,
+    "search/issues", "search/prs",
+    "Issue search results", "PR search results",
+  );
 }
 
 /**
@@ -1207,21 +1224,11 @@ async function graphqlUnfilteredSearch(
     }
   }));
 
-  if (issues.length >= SEARCH_RESULT_CAP) {
-    console.warn(`[api] Unfiltered issue results capped at ${SEARCH_RESULT_CAP}`);
-    pushNotification("search/unfiltered-issues", `Monitored repo issue results capped at ${SEARCH_RESULT_CAP.toLocaleString("en-US")} — some items are hidden`, "warning");
-    issues.splice(SEARCH_RESULT_CAP);
-  }
-
-  if (prMap.size >= SEARCH_RESULT_CAP) {
-    console.warn(`[api] Unfiltered PR results capped at ${SEARCH_RESULT_CAP}`);
-    pushNotification("search/unfiltered-prs", `Monitored repo PR results capped at ${SEARCH_RESULT_CAP.toLocaleString("en-US")} — some items are hidden`, "warning");
-  }
-
-  const pullRequests = [...prMap.values()];
-  if (pullRequests.length >= SEARCH_RESULT_CAP) pullRequests.splice(SEARCH_RESULT_CAP);
-  const prNodeIds = pullRequests.map((pr) => nodeIdMap.get(pr.id)).filter((id): id is string => id != null);
-  return { issues, pullRequests, prNodeIds, errors };
+  return finalizeSearchResult(
+    issues, prMap, nodeIdMap, errors,
+    "search/unfiltered-issues", "search/unfiltered-prs",
+    "Monitored repo issue results", "Monitored repo PR results",
+  );
 }
 
 // ── Two-phase: heavy backfill ─────────────────────────────────────────────────
@@ -2210,7 +2217,7 @@ export async function validateGitHubUser(
     login: raw.login.toLowerCase(),
     avatarUrl,
     name: raw.name ?? null,
-    type: raw.type === "Bot" ? "bot" as const : "user" as const,
+    type: raw.type === "Bot" ? "bot" : "user",
   };
 }
 
diff --git a/tests/services/api.test.ts b/tests/services/api.test.ts
@@ -475,7 +475,7 @@ describe("fetchIssues", () => {
     const result = await fetchIssues(
       octokit as never,
       [testRepo],
-      "bad user" // contains space — fails VALID_LOGIN
+      "bad user" // contains space — fails VALID_TRACKED_LOGIN
     );
 
     expect(result.issues).toEqual([]);
@@ -1904,6 +1904,78 @@ describe("fetchIssuesAndPullRequests — unfiltered search error handling", () =
     expect(result.errors.length).toBeGreaterThan(0);
     expect(result.errors[0]).toMatchObject({ retryable: true });
   });
+
+  it("recovers PR data when issues is null but prs has data (symmetric partial case)", async () => {
+    const monitoredRepo = { owner: "org", name: "monitored", fullName: "org/monitored" };
+    const prNode = {
+      id: "PR_kwDOtest4501",
+      databaseId: 4501,
+      number: 1,
+      title: "Partial PR",
+      state: "open",
+      url: "https://github.com/org/monitored/pull/1",
+      createdAt: "2024-01-01T00:00:00Z",
+      updatedAt: "2024-01-02T00:00:00Z",
+      author: { login: "someone", avatarUrl: "https://avatars.githubusercontent.com/u/1" },
+      labels: { nodes: [] },
+      assignees: { nodes: [] },
+      repository: { nameWithOwner: "org/monitored" },
+      additions: 10,
+      deletions: 5,
+      changedFiles: 2,
+      headRefName: "fix/thing",
+      baseRefName: "main",
+      isDraft: false,
+      reviewDecision: null,
+      commits: { nodes: [{ commit: { statusCheckRollup: null } }] },
+      headRepository: { owner: { login: "org" } },
+      latestReviews: { nodes: [] },
+      reviewThreads: { totalCount: 0 },
+    };
+
+    let callCount = 0;
+    const octokit = makeOctokit(
+      async () => ({ data: {}, headers: {} }),
+      async () => {
+        callCount++;
+        if (callCount === 1) {
+          const err = Object.assign(new Error("Partial failure"), {
+            data: {
+              issues: null,
+              prs: { issueCount: 1, pageInfo: { hasNextPage: false, endCursor: null }, nodes: [prNode] },
+              rateLimit: { limit: 5000, remaining: 4998, resetAt: new Date(Date.now() + 3600000).toISOString() },
+            },
+          });
+          throw err;
+        }
+        return {
+          issues: { issueCount: 0, pageInfo: { hasNextPage: false, endCursor: null }, nodes: [] },
+          prs: { issueCount: 0, pageInfo: { hasNextPage: false, endCursor: null }, nodes: [] },
+          prInvolves: { issueCount: 0, pageInfo: { hasNextPage: false, endCursor: null }, nodes: [] },
+          prReviewReq: { issueCount: 0, pageInfo: { hasNextPage: false, endCursor: null }, nodes: [] },
+          rateLimit: { limit: 5000, remaining: 4999, resetAt: new Date(Date.now() + 3600000).toISOString() },
+        };
+      }
+    );
+
+    const { fetchIssuesAndPullRequests } = await import("../../src/app/services/api");
+    const result = await fetchIssuesAndPullRequests(
+      octokit as never,
+      [monitoredRepo],
+      "octocat",
+      undefined,
+      undefined,
+      [{ owner: "org", name: "monitored", fullName: "org/monitored" }]
+    );
+
+    // PR data recovered from partial response
+    expect(result.pullRequests).toHaveLength(1);
+    expect(result.pullRequests[0].id).toBe(4501);
+    // Issues empty (null coalesced to empty)
+    expect(result.issues).toHaveLength(0);
+    // Error recorded
+    expect(result.errors.some(e => e.retryable)).toBe(true);
+  });
 });
 
 describe("fetchIssuesAndPullRequests — all monitored + tracked users skips involves: search", () => {
diff --git a/tests/stores/auth.test.ts b/tests/stores/auth.test.ts
@@ -5,6 +5,11 @@ vi.mock("../../src/app/stores/cache", () => ({
   clearCache: vi.fn().mockResolvedValue(undefined),
 }));
 
+const mockPushNotification = vi.fn();
+vi.mock("../../src/app/lib/errors", () => ({
+  pushNotification: (...args: unknown[]) => mockPushNotification(...args),
+}));
+
 // Mock localStorage with full control (same pattern as config.test.ts)
 const localStorageMock = (() => {
   let store: Record<string, string> = {};
@@ -68,6 +73,24 @@ describe("setAuth / token signal", () => {
     const fresh = await import("../../src/app/stores/auth");
     expect(fresh.token()).toBe("ghs_persisted");
   });
+
+  it("sets token in memory and warns when localStorage.setItem throws", () => {
+    const origSetItem = localStorageMock.setItem;
+    localStorageMock.setItem = () => { throw new DOMException("QuotaExceededError"); };
+    mockPushNotification.mockClear();
+
+    try {
+      mod.setAuth({ access_token: "ghs_quota" });
+      expect(mod.token()).toBe("ghs_quota");
+      expect(mockPushNotification).toHaveBeenCalledWith(
+        "localStorage:auth",
+        expect.stringContaining("storage may be full"),
+        "warning",
+      );
+    } finally {
+      localStorageMock.setItem = origSetItem;
+    }
+  });
 });
 
 describe("isAuthenticated", () => {
@@ -351,6 +374,26 @@ describe("validateToken", () => {
     expect(mod.token()).toBe("ghs_retrynet");
   });
 
+  it("does not invalidate a new token set during the retry window (race condition guard)", async () => {
+    vi.stubGlobal("fetch", vi.fn()
+      // First GET /user returns 401
+      .mockResolvedValueOnce({ ok: false, status: 401, json: () => Promise.resolve({}) })
+      // Retry also returns 401 — but token was replaced mid-window
+      .mockResolvedValueOnce({ ok: false, status: 401, json: () => Promise.resolve({}) })
+    );
+
+    mod.setAuth({ access_token: "ghs_old" });
+    const promise = mod.validateToken();
+    // Simulate user re-authenticating during the 1-second retry delay
+    mod.setAuth({ access_token: "ghs_new" });
+    await vi.advanceTimersByTimeAsync(1000);
+    const result = await promise;
+    expect(result).toBe(false);
+    // The NEW token must survive — guard prevents expireToken() from running
+    expect(mod.token()).toBe("ghs_new");
+    expect(localStorageMock.getItem("github-tracker:auth-token")).toBe("ghs_new");
+  });
+
   it("preserves user config and view state when token expires (not a full logout)", async () => {
     vi.stubGlobal("fetch", vi.fn()
       .mockResolvedValueOnce({ ok: false, status: 401, json: () => Promise.resolve({}) })
