fix: addresses PR review findings across sort, perf, and tests

Author: wgordon17

src/app/components/onboarding/RepoSelector.tsx

@@ -153,9 +153,15 @@ export default function RepoSelector(props: RepoSelectorProps) {
     // After initial load (all orgs resolved), sorting stays active during retries
     // because loadedCount is not reset by retryOrg.
     if (loadedCount() < props.selectedOrgs.length) return states;
+    const maxPushedAt = new Map(
+      states.map((s) => [
+        s.org,
+        s.repos.reduce((max, r) => r.pushedAt && r.pushedAt > max ? r.pushedAt : max, ""),
+      ])
+    );
     return [...states].sort((a, b) => {
-      const aMax = a.repos.reduce((max, r) => r.pushedAt && r.pushedAt > max ? r.pushedAt : max, "");
-      const bMax = b.repos.reduce((max, r) => r.pushedAt && r.pushedAt > max ? r.pushedAt : max, "");
+      const aMax = maxPushedAt.get(a.org) ?? "";
+      const bMax = maxPushedAt.get(b.org) ?? "";
       return aMax > bMax ? -1 : aMax < bMax ? 1 : 0;
     });
   });
@@ -272,7 +278,7 @@ export default function RepoSelector(props: RepoSelectorProps) {
       {/* Per-org repo lists */}
       <For each={sortedOrgStates()}>
         {(state) => {
-          const visible = () => filteredReposForOrg(state);
+          const visible = createMemo(() => filteredReposForOrg(state));
 
           return (
             <div class="overflow-hidden rounded-lg border border-gray-200 dark:border-gray-700">

src/app/services/api.ts

@@ -1037,14 +1037,14 @@ export async function fetchWorkflowRuns(
       runs,
       latestAt: runs.reduce((max, r) => r.updated_at > max ? r.updated_at : max, ""),
     }));
-    workflowEntries.sort((a, b) => b.latestAt > a.latestAt ? -1 : b.latestAt < a.latestAt ? 1 : 0);
+    workflowEntries.sort((a, b) => a.latestAt > b.latestAt ? -1 : a.latestAt < b.latestAt ? 1 : 0);
     const topWorkflows = workflowEntries
       .slice(0, maxWorkflows);
 
     // Take most recent M runs per workflow
     for (const { runs: workflowRuns } of topWorkflows) {
       const sorted = workflowRuns.sort(
-        (a, b) => b.created_at > a.created_at ? -1 : b.created_at < a.created_at ? 1 : 0
+        (a, b) => a.created_at > b.created_at ? -1 : a.created_at < b.created_at ? 1 : 0
       );
       for (const run of sorted.slice(0, maxRuns)) {
         allRuns.push({

tests/components/onboarding/RepoSelector.test.tsx

@@ -1,5 +1,5 @@
 import { describe, it, expect, vi, beforeEach } from "vitest";
-import { render, screen, fireEvent, waitFor } from "@solidjs/testing-library";
+import { render, screen, waitFor } from "@solidjs/testing-library";
 import userEvent from "@testing-library/user-event";
 import type { RepoRef, RepoEntry } from "../../../src/app/services/api";
 
@@ -38,6 +38,7 @@ const otherorgRepos: RepoEntry[] = [
 describe("RepoSelector", () => {
   beforeEach(() => {
     vi.clearAllMocks();
+    vi.restoreAllMocks();
   });
 
   it("shows loading while fetching repos", async () => {
@@ -100,6 +101,7 @@ describe("RepoSelector", () => {
   });
 
   it("filters repos by text input", async () => {
+    const user = userEvent.setup();
     vi.mocked(api.fetchRepos).mockResolvedValue(myorgRepos);
 
     render(() => (
@@ -111,7 +113,7 @@ describe("RepoSelector", () => {
     });
 
     const filterInput = screen.getByPlaceholderText(/Filter repos/i);
-    fireEvent.input(filterInput, { target: { value: "repo-a" } });
+    await user.type(filterInput, "repo-a");
 
     await waitFor(() => {
       screen.getByText("repo-a");
@@ -132,9 +134,8 @@ describe("RepoSelector", () => {
       screen.getByText("repo-a");
     });
 
-    // "Select All" button in the org header (there may be multiple — use the first one)
+    // With a single org: [global Select All, per-org Select All] — click the per-org (last) one
     const selectAllBtns = screen.getAllByText("Select All");
-    // The per-org one is inside the org group; for a single org there's only one
     await user.click(selectAllBtns[selectAllBtns.length - 1]);
 
     expect(onChange).toHaveBeenCalled();
@@ -215,13 +216,14 @@ describe("RepoSelector", () => {
   });
 
   it("shows relative time next to each repo", async () => {
+    vi.spyOn(Date, "now").mockReturnValue(new Date("2026-03-24T12:00:00Z").getTime());
     vi.mocked(api.fetchRepos).mockResolvedValue(myorgRepos);
     render(() => (
       <RepoSelector selectedOrgs={["myorg"]} selected={[]} onChange={vi.fn()} />
     ));
     await waitFor(() => {
-      const labels = screen.getAllByText(/ago|yesterday|just now|last/i);
-      expect(labels.length).toBeGreaterThanOrEqual(2);
+      screen.getByText("4 days ago");
+      screen.getByText("2 days ago");
     });
   });
 
@@ -261,4 +263,53 @@ describe("RepoSelector", () => {
     });
     expect(screen.queryByText(/ago|yesterday|just now|last/i)).toBeNull();
   });
+
+  it("global Select All strips pushedAt from onChange payload", async () => {
+    const user = userEvent.setup();
+    vi.mocked(api.fetchRepos).mockImplementation((_client, org) => {
+      if (org === "myorg") return Promise.resolve(myorgRepos);
+      return Promise.resolve(otherorgRepos);
+    });
+    const onChange = vi.fn();
+    render(() => (
+      <RepoSelector selectedOrgs={["myorg", "otherog"]} selected={[]} onChange={onChange} />
+    ));
+    await waitFor(() => {
+      screen.getByText("repo-a");
+      screen.getByText("repo-c");
+    });
+    // The first "Select All" button is the global one in the header
+    const selectAllBtns = screen.getAllByText("Select All");
+    await user.click(selectAllBtns[0]);
+    expect(onChange).toHaveBeenCalled();
+    const result = onChange.mock.calls[0][0] as RepoRef[];
+    expect(result.length).toBe(3);
+    for (const r of result) {
+      expect(r).not.toHaveProperty("pushedAt");
+    }
+  });
+
+  it("preserves org order when all repos have null pushedAt", async () => {
+    const nullOrg1: RepoEntry[] = [
+      { owner: "stale-org", name: "null-repo-1", fullName: "stale-org/null-repo-1", pushedAt: null },
+    ];
+    const nullOrg2: RepoEntry[] = [
+      { owner: "active-org", name: "null-repo-2", fullName: "active-org/null-repo-2", pushedAt: null },
+    ];
+    vi.mocked(api.fetchRepos).mockImplementation((_client, org) => {
+      if (org === "stale-org") return Promise.resolve(nullOrg1);
+      return Promise.resolve(nullOrg2);
+    });
+    render(() => (
+      <RepoSelector selectedOrgs={["stale-org", "active-org"]} selected={[]} onChange={vi.fn()} />
+    ));
+    await waitFor(() => {
+      screen.getByText("null-repo-1");
+      screen.getByText("null-repo-2");
+    });
+    const orgHeaders = screen.getAllByText(/^(active-org|stale-org)$/);
+    // Both have null pushedAt → comparator returns 0 → original order preserved
+    expect(orgHeaders[0].textContent).toBe("stale-org");
+    expect(orgHeaders[1].textContent).toBe("active-org");
+  });
 });

tests/services/api.test.ts

@@ -720,6 +720,38 @@ describe("fetchWorkflowRuns", () => {
       fetchWorkflowRuns(null, [testRepo], 5, 3)
     ).rejects.toThrow("No GitHub client available");
   });
+
+  it("returns runs sorted newest-first within each workflow", async () => {
+    const octokit = makeOctokitForRuns();
+
+    const { workflowRuns } = await fetchWorkflowRuns(
+      octokit as unknown as ReturnType<typeof import("../../src/app/services/github").getClient>,
+      [testRepo],
+      5,
+      10
+    );
+
+    // Workflow 1001 has 3 runs: 9002 (10:00), 9001 (09:00), 9003 (14:15:00 prev day)
+    const w1001Runs = workflowRuns.filter((r) => r.workflowId === 1001);
+    for (let i = 1; i < w1001Runs.length; i++) {
+      expect(w1001Runs[i - 1].createdAt >= w1001Runs[i].createdAt).toBe(true);
+    }
+  });
+
+  it("selects workflows with most recent activity first", async () => {
+    const octokit = makeOctokitForRuns();
+
+    const { workflowRuns } = await fetchWorkflowRuns(
+      octokit as unknown as ReturnType<typeof import("../../src/app/services/github").getClient>,
+      [testRepo],
+      5,
+      10
+    );
+
+    // First run in results should be from the workflow with the most recent updatedAt
+    // Workflow 1001 latestAt=2024-01-15T10:05:00Z > Workflow 1002 latestAt=2024-01-15T09:25:00Z
+    expect(workflowRuns[0].workflowId).toBe(1001);
+  });
 });
 
 // ── searchAllPages pagination ─────────────────────────────────────────────────