Merge pull request #686 from nazarli-shabnam/fix/security-owner-change-549

fix(ui): clear the Security page's scan results and selection when the owner changes

Author: nazarli-shabnam

apps/ui/app/security/page.tsx

@@ -167,9 +167,9 @@ export default function SecurityPage() {
   })
 
   const scan = useMutation({
-    mutationFn: () => api.analytics.overview(owner, token),
-    onSuccess: () => {
-      queryClient.invalidateQueries({ queryKey: ["analytics.history", owner] })
+    mutationFn: (forOwner: string) => api.analytics.overview(forOwner, token),
+    onSuccess: (_data, forOwner) => {
+      queryClient.invalidateQueries({ queryKey: ["analytics.history", forOwner] })
     },
   })
 
@@ -199,14 +199,30 @@ export default function SecurityPage() {
 
   const [selectedRepo, setSelectedRepo] = useState("")
   const matrixMutation = useMutation({
-    mutationFn: () => api.security.matrix(owner, token),
+    mutationFn: (forOwner: string) => api.security.matrix(forOwner, token),
     onSuccess: (data) => setSelectedRepo(data.repos[0]?.repo ?? ""),
   })
 
+  // The render right after an owner change still holds the previous owner's results until the reset
+  // effect below runs; these keep them off the screen for that render instead of relying on the effect.
+  const scanIsForOwner = scan.variables === owner
+  const matrixIsForOwner = matrixMutation.variables === owner
+
+  // Results belong to the owner they were scanned for: drop them (and the repo picked from them)
+  // when the owner changes, or the old org's data stays up under the new name and keeps querying.
+  useEffect(() => {
+    scan.reset()
+    matrixMutation.reset()
+    setSelectedRepo("")
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [owner])
+
   const secretScanning = useQuery({
     queryKey: ["security.secret-scanning", owner, selectedRepo],
     queryFn: () => api.security.secretScanning(owner, selectedRepo, token),
-    enabled: !!selectedRepo && !!matrixMutation.data,
+    // variables === owner: the render right after an owner change still holds the old matrix and repo
+    // until the reset effect runs; without this it would fire one request for new-owner/old-repo.
+    enabled: !!selectedRepo && !!matrixMutation.data && matrixMutation.variables === owner,
   })
 
   // A failed history fetch must not look like "never scanned": the trend sections would otherwise
@@ -245,8 +261,8 @@ export default function SecurityPage() {
   }))
 
   function runScan() {
-    scan.mutate()
-    matrixMutation.mutate()
+    scan.mutate(owner)
+    matrixMutation.mutate(owner)
   }
 
   const filteredChecks = scan.data
@@ -350,7 +366,7 @@ export default function SecurityPage() {
                 {saveTokenMutation.error.message}
               </div>
             )}
-            {scan.isError && (
+            {scan.isError && scanIsForOwner && (
               <div data-testid="scan-error" className="flex items-start gap-2 text-xs text-destructive">
                 <Warning className="size-3.5 mt-0.5 shrink-0" />
                 {scan.error.message}
@@ -404,7 +420,7 @@ export default function SecurityPage() {
           </div>
         </div>
 
-        {(scan.data || scan.isPending) && (
+        {scanIsForOwner && (scan.data || scan.isPending) && (
           <div className="card lg:col-span-2">
             {scan.data && (
               <div className="px-4 py-3 border-b border-border">
@@ -511,7 +527,7 @@ export default function SecurityPage() {
         )}
       </div>
 
-      {(matrixMutation.data || matrixMutation.isPending || matrixMutation.error) && (
+      {matrixIsForOwner && (matrixMutation.data || matrixMutation.isPending || matrixMutation.error) && (
         <div className="grid gap-4 lg:grid-cols-2 mt-6">
           <div className="card">
             <div className="px-4 py-3 border-b border-border flex items-center justify-between">

apps/ui/tests/components/security-page.test.tsx

@@ -177,6 +177,66 @@ describe("SecurityPage", () => {
     await waitFor(() => expect(analyticsOverviewMock).toHaveBeenCalledWith("acme", ""));
   });
 
+  it("drops the previous owner's scan results and stops querying for them when the owner changes (#549)", async () => {
+    analyticsOverviewMock.mockResolvedValue({
+      owner: "acme", score: 50, total_checks: 1, failed_checks: 1, repo_count: 1,
+      checks: [{ id: "c1", title: "Acme-only check", severity: "high", remediation: "", status: "fail", value: { type: "boolean", enabled: false } }],
+    });
+    securityMatrixMock.mockResolvedValue({
+      owner: "acme",
+      repos: [{
+        repo: "acme-repo", branch_protection: true, secret_scanning: true, dependabot_enabled: true,
+        dependabot_critical_count: 0, dependabot_high_count: 0, code_scanning: true, force_push_allowed: false,
+        score: 100, unknown_dimensions: [], alerts_source: "github",
+      }],
+      summary: { fully_compliant_count: 1, critical_risk_count: 0, secret_hits_count: 0, vuln_by_severity: { critical: 0, high: 0, medium: 0, low: 0 } },
+    });
+
+    renderPage();
+    const ownerInput = screen.getByPlaceholderText("e.g. octocat");
+    fireEvent.change(ownerInput, { target: { value: "acme" } });
+    const scanButton = screen.getByRole("button", { name: /run scan/i });
+    await waitFor(() => expect(scanButton).not.toBeDisabled());
+    fireEvent.click(scanButton);
+    expect(await screen.findByText("Acme-only check")).toBeInTheDocument();
+    await waitFor(() => expect(secretScanningMock).toHaveBeenCalledWith("acme", "acme-repo", ""));
+
+    secretScanningMock.mockClear();
+    fireEvent.change(ownerInput, { target: { value: "beta" } });
+
+    await waitFor(() => expect(screen.queryByText("Acme-only check")).not.toBeInTheDocument());
+    expect(secretScanningMock).not.toHaveBeenCalled();
+  });
+
+  it("shows a scan failure for the owner it was run for, and drops it once the owner changes (#549)", async () => {
+    analyticsOverviewMock.mockRejectedValue(new Error("scan exploded"));
+
+    renderPage();
+    const ownerInput = screen.getByPlaceholderText("e.g. octocat");
+    fireEvent.change(ownerInput, { target: { value: "acme" } });
+    const scanButton = screen.getByRole("button", { name: /run scan/i });
+    await waitFor(() => expect(scanButton).not.toBeDisabled());
+    fireEvent.click(scanButton);
+
+    expect(await screen.findByTestId("scan-error")).toHaveTextContent("scan exploded");
+
+    fireEvent.change(ownerInput, { target: { value: "beta" } });
+    await waitFor(() => expect(screen.queryByTestId("scan-error")).not.toBeInTheDocument());
+  });
+
+  it("shows the results card with a pending state while a scan is running (#549)", async () => {
+    analyticsOverviewMock.mockReturnValue(new Promise(() => {})); // never settles
+
+    renderPage();
+    fireEvent.change(screen.getByPlaceholderText("e.g. octocat"), { target: { value: "acme" } });
+    const scanButton = screen.getByRole("button", { name: /run scan/i });
+    await waitFor(() => expect(scanButton).not.toBeDisabled());
+    fireEvent.click(scanButton);
+
+    await waitFor(() => expect(screen.getByRole("button", { name: /scanning/i })).toBeInTheDocument());
+    expect(analyticsOverviewMock).toHaveBeenCalledWith("acme", "");
+  });
+
   it("runs a scan on Enter in the organization field with no token entered", async () => {
     analyticsOverviewMock.mockResolvedValue({
       owner: "acme",