fix(hot-poll): addresses all PR review findings

Author: wgordon17

src/app/components/dashboard/DashboardPage.tsx

@@ -24,6 +24,9 @@ import { getClient, getGraphqlRateLimit } from "../../services/github";
 
 // ── Shared dashboard store (module-level to survive navigation) ─────────────
 
+// Bump only for breaking schema changes (renames, type changes). Additive optional
+// fields (e.g., nodeId?: string) don't require a bump — missing fields deserialize
+// as undefined, which consuming code handles gracefully.
 const CACHE_VERSION = 2;
 
 interface DashboardStore {
@@ -240,7 +243,6 @@ export default function DashboardPage() {
       _setHotCoordinator(createHotPollCoordinator(
         () => config.hotPollInterval,
         (prUpdates, runUpdates, fetchGeneration) => {
-          if (prUpdates.size === 0 && runUpdates.size === 0) return;
           // Guard against stale hot poll results overlapping with a full refresh.
           // fetchGeneration was captured BEFORE fetchHotData() started its async work.
           // If a full refresh completed during the fetch, _hotPollGeneration will have

src/app/services/api.ts

@@ -1834,12 +1834,12 @@ export async function fetchWorkflowRunById(
     run_id: id,
   });
   updateRateLimitFromHeaders(response.headers as Record<string, string>);
-  const run = response.data;
+  const run = response.data as unknown as RawWorkflowRun;
   return {
     id: run.id,
     status: run.status ?? "",
     conclusion: run.conclusion ?? null,
     updatedAt: run.updated_at,
-    completedAt: ("completed_at" in run ? (run as Record<string, unknown>).completed_at as string | null : null) ?? null,
+    completedAt: run.completed_at ?? null,
   };
 }

src/app/services/poll.ts

@@ -46,11 +46,14 @@ let _notifGateDisabled = false; // Disabled after 403 (notifications scope not g
 
 /** PRs with pending/null check status: maps GraphQL node ID → databaseId */
 const _hotPRs = new Map<string, number>();
+/** Inverse index for O(1) eviction: maps databaseId → nodeId */
+const _hotPRsByDbId = new Map<number, string>();
 const MAX_HOT_PRS = 200;
 
 /** In-progress workflow runs: maps run ID → repo descriptor */
 const _hotRuns = new Map<number, { owner: string; repo: string }>();
 const MAX_HOT_RUNS = 30;
+const HOT_RUNS_CONCURRENCY = 10;
 
 /** Incremented each time rebuildHotSets() is called (full refresh completed).
  * Allows hot poll callbacks to detect stale results that overlap with a fresh
@@ -64,6 +67,7 @@ export function getHotPollGeneration(): number {
 
 export function clearHotSets(): void {
   _hotPRs.clear();
+  _hotPRsByDbId.clear();
   _hotRuns.clear();
 }
 
@@ -72,6 +76,7 @@ export function resetPollState(): void {
   _lastSuccessfulFetch = null;
   _notifGateDisabled = false;
   _hotPRs.clear();
+  _hotPRsByDbId.clear();
   _hotRuns.clear();
   _hotPollGeneration = 0;
   _resetNotificationState();
@@ -391,6 +396,7 @@ export function createPollCoordinator(
 export function rebuildHotSets(data: DashboardData): void {
   _hotPollGeneration++;
   _hotPRs.clear();
+  _hotPRsByDbId.clear();
   _hotRuns.clear();
 
   for (const pr of data.pullRequests) {
@@ -400,6 +406,7 @@ export function rebuildHotSets(data: DashboardData): void {
         break;
       }
       _hotPRs.set(pr.nodeId, pr.id);
+      _hotPRsByDbId.set(pr.id, pr.nodeId);
     }
   }
 
@@ -459,9 +466,9 @@ export async function fetchHotData(): Promise<{
   // Workflow run fetches — bounded concurrency via pooledAllSettled
   const runEntries = [..._hotRuns.entries()];
   const runTasks = runEntries.map(
-    (entry) => async () => fetchWorkflowRunById(octokit, { id: entry[0], ...entry[1] })
+    ([runId, descriptor]) => async () => fetchWorkflowRunById(octokit, { id: runId, ...descriptor })
   );
-  const runResults = await pooledAllSettled(runTasks, 10);
+  const runResults = await pooledAllSettled(runTasks, HOT_RUNS_CONCURRENCY);
   for (const result of runResults) {
     if (result.status === "fulfilled") {
       runUpdates.set(result.value.id, result.value);
@@ -474,18 +481,17 @@ export async function fetchHotData(): Promise<{
   // The freshly rebuilt sets are authoritative — evicting from them based on
   // stale fetch results would corrupt the new data.
   if (generation === _hotPollGeneration) {
-    // Evict settled PRs
+    // Evict settled PRs using inverse index for O(1) lookup
     for (const [databaseId, upd] of prUpdates) {
       if (
         upd.state === "CLOSED" ||
         upd.state === "MERGED" ||
         (upd.checkStatus !== "pending" && upd.checkStatus !== null)
       ) {
-        for (const [nodeId, id] of _hotPRs) {
-          if (id === databaseId) {
-            _hotPRs.delete(nodeId);
-            break;
-          }
+        const nodeId = _hotPRsByDbId.get(databaseId);
+        if (nodeId) {
+          _hotPRs.delete(nodeId);
+          _hotPRsByDbId.delete(databaseId);
         }
       }
     }
@@ -525,6 +531,7 @@ export function createHotPollCoordinator(
   const MAX_BACKOFF_MULTIPLIER = 8; // caps at 8× the base interval
 
   function destroy(): void {
+    // Invalidates any in-flight cycle(); createEffect captures the new value as the next chain's seed
     chainGeneration++;
     consecutiveFailures = 0;
     if (timeoutId !== null) {
@@ -548,6 +555,12 @@ export function createHotPollCoordinator(
       return;
     }
 
+    // Skip fetch when no authenticated client (e.g., mid-logout)
+    if (!getClient()) {
+      schedule(myGeneration);
+      return;
+    }
+
     try {
       const { prUpdates, runUpdates, generation, hadErrors } = await fetchHotData();
       if (myGeneration !== chainGeneration) return; // Chain destroyed during fetch
@@ -556,10 +569,13 @@ export function createHotPollCoordinator(
       } else {
         consecutiveFailures = 0;
       }
-      onHotData(prUpdates, runUpdates, generation);
+      if (prUpdates.size > 0 || runUpdates.size > 0) {
+        onHotData(prUpdates, runUpdates, generation);
+      }
     } catch (err) {
       consecutiveFailures++;
-      console.warn(`[hot-poll] cycle failed (${consecutiveFailures}x):`, err);
+      const message = err instanceof Error ? err.message : "Unknown hot-poll error";
+      pushError("hot-poll", message, true);
     }
 
     schedule(myGeneration);

src/app/stores/config.ts

@@ -71,9 +71,12 @@ export function loadConfig(): Config {
 export const [config, setConfig] = createStore<Config>(loadConfig());
 
 export function updateConfig(partial: Partial<Config>): void {
+  // Validate partial update through Zod to enforce schema bounds (e.g., min/max)
+  const validated = ConfigSchema.partial().safeParse(partial);
+  const safe = validated.success ? validated.data : partial;
   setConfig(
     produce((draft) => {
-      Object.assign(draft, partial);
+      Object.assign(draft, safe);
     })
   );
 }

tests/components/DashboardPage.test.tsx

@@ -59,6 +59,12 @@ vi.mock("../../src/app/lib/errors", () => ({
 // capturedFetchAll is populated by the createPollCoordinator mock each time
 // the module is reset and DashboardPage re-mounts, creating a fresh coordinator.
 let capturedFetchAll: (() => Promise<DashboardData>) | null = null;
+// capturedOnHotData is populated by the createHotPollCoordinator mock
+let capturedOnHotData: ((
+  prUpdates: Map<number, { state: string; checkStatus: string; mergeStateStatus: string; reviewDecision: string | null }>,
+  runUpdates: Map<number, { id: number; status: string; conclusion: string | null; updatedAt: string; completedAt: string | null }>,
+  generation: number,
+) => void) | null = null;
 
 // DashboardPage and pollService are imported dynamically after each vi.resetModules()
 // so the module-level _coordinator variable is always fresh (null) per test.
@@ -99,7 +105,12 @@ beforeEach(async () => {
         };
       }
     ),
-    createHotPollCoordinator: vi.fn().mockReturnValue({ destroy: vi.fn() }),
+    createHotPollCoordinator: vi.fn().mockImplementation(
+      (_getInterval: unknown, onHotData: typeof capturedOnHotData) => {
+        capturedOnHotData = onHotData;
+        return { destroy: vi.fn() };
+      }
+    ),
     rebuildHotSets: vi.fn(),
     clearHotSets: vi.fn(),
     getHotPollGeneration: vi.fn().mockReturnValue(0),
@@ -113,6 +124,7 @@ beforeEach(async () => {
 
   mockLocationReplace.mockClear();
   capturedFetchAll = null;
+  capturedOnHotData = null;
   vi.mocked(authStore.clearAuth).mockClear();
   vi.mocked(pollService.fetchAllData).mockResolvedValue({
     issues: [],
@@ -352,3 +364,82 @@ describe("DashboardPage — onAuthCleared integration", () => {
     });
   });
 });
+
+describe("DashboardPage — onHotData integration", () => {
+  it("applies hot poll PR status updates to the store", async () => {
+    const testPR = makePullRequest({
+      id: 42,
+      checkStatus: "pending",
+      state: "open",
+      reviewDecision: null,
+    });
+    vi.mocked(pollService.fetchAllData).mockResolvedValue({
+      issues: [],
+      pullRequests: [testPR],
+      workflowRuns: [],
+      errors: [],
+    });
+    render(() => <DashboardPage />);
+    await waitFor(() => {
+      expect(capturedOnHotData).not.toBeNull();
+    });
+
+    // Verify initial state shows pending
+    const user = userEvent.setup();
+    await user.click(screen.getByText("Pull Requests"));
+    await waitFor(() => {
+      expect(screen.getByLabelText("Checks in progress")).toBeTruthy();
+    });
+
+    // Simulate hot poll returning a status update (generation=0 matches default mock)
+    const prUpdates = new Map([[42, {
+      state: "OPEN",
+      checkStatus: "success" as const,
+      mergeStateStatus: "CLEAN",
+      reviewDecision: "APPROVED" as const,
+    }]]);
+    capturedOnHotData!(prUpdates, new Map(), 0);
+
+    // The StatusDot should update from "Checks in progress" to "All checks passed"
+    await waitFor(() => {
+      expect(screen.getByLabelText("All checks passed")).toBeTruthy();
+    });
+  });
+
+  it("discards stale hot poll updates when generation mismatches", async () => {
+    const testPR = makePullRequest({
+      id: 43,
+      checkStatus: "pending",
+      state: "open",
+    });
+    vi.mocked(pollService.fetchAllData).mockResolvedValue({
+      issues: [],
+      pullRequests: [testPR],
+      workflowRuns: [],
+      errors: [],
+    });
+    render(() => <DashboardPage />);
+    await waitFor(() => {
+      expect(capturedOnHotData).not.toBeNull();
+    });
+
+    const user = userEvent.setup();
+    await user.click(screen.getByText("Pull Requests"));
+    await waitFor(() => {
+      expect(screen.getByLabelText("Checks in progress")).toBeTruthy();
+    });
+
+    // Send update with stale generation (999 !== mock default of 0)
+    const prUpdates = new Map([[43, {
+      state: "OPEN",
+      checkStatus: "success" as const,
+      mergeStateStatus: "CLEAN",
+      reviewDecision: null,
+    }]]);
+    capturedOnHotData!(prUpdates, new Map(), 999);
+
+    // PR should still show pending — stale update was discarded
+    expect(screen.getByLabelText("Checks in progress")).toBeTruthy();
+    expect(screen.queryByLabelText("All checks passed")).toBeNull();
+  });
+});

tests/lib/notifications.test.ts

@@ -6,34 +6,16 @@ import {
   _resetNotificationState,
   type NewItems,
 } from "../../src/app/lib/notifications";
-import type { Config } from "../../src/app/stores/config";
+import { ConfigSchema, type Config } from "../../src/app/stores/config";
 import type { DashboardData } from "../../src/app/services/poll";
 import type { Issue, PullRequest, WorkflowRun } from "../../src/app/services/api";
 
 // ── Fixtures ──────────────────────────────────────────────────────────────────
 
 function makeConfig(overrides: Partial<Config["notifications"]> = {}): Config {
-  return {
-    selectedOrgs: [],
-    selectedRepos: [],
-    refreshInterval: 300,
-    hotPollInterval: 30,
-    maxWorkflowsPerRepo: 5,
-    maxRunsPerWorkflow: 3,
-    notifications: {
-      enabled: true,
-      issues: true,
-      pullRequests: true,
-      workflowRuns: true,
-      ...overrides,
-    },
-    theme: "light",
-    viewDensity: "comfortable",
-    itemsPerPage: 25,
-    defaultTab: "issues",
-    rememberLastTab: true,
-    onboardingComplete: false,
-  };
+  return ConfigSchema.parse({
+    notifications: { enabled: true, ...overrides },
+  });
 }
 
 function makeIssue(id: number): Issue {