Commit ·
d0b1ea0
1
Parent(s): cfeab47
fix: CI failures + ironclad pre-commit hooks
Browse filesFixes CI failures from CodeRabbit feedback PR:
1. ESLint react-hooks/set-state-in-effect error:
- Restructured CaseSelector to use inline async function pattern
- Moved fetch logic inside useEffect (React docs recommended pattern)
- Added isActive guard for cleanup race conditions
2. Coverage threshold (69.23% < 70%):
- Added test for 503 cold-start retry scenario
- Added casesColdStart mock handler with retry counter
- Coverage now 71% (70 tests passing)
3. Added ironclad pre-commit hooks for frontend:
- frontend-lint: ESLint on commit
- frontend-typecheck: TypeScript on commit
- frontend-test: Coverage tests on push (slower, so push-only)
This ensures CI failures are caught locally before push.
.pre-commit-config.yaml
CHANGED
|
@@ -1,4 +1,7 @@
|
|
| 1 |
repos:
|
|
|
|
|
|
|
|
|
|
| 2 |
- repo: https://github.com/astral-sh/ruff-pre-commit
|
| 3 |
rev: v0.14.8
|
| 4 |
hooks:
|
|
@@ -17,6 +20,36 @@ repos:
|
|
| 17 |
# Exclude auto-generated Gradio custom component files
|
| 18 |
exclude: ^packages/niivueviewer/
|
| 19 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 20 |
- repo: https://github.com/pre-commit/pre-commit-hooks
|
| 21 |
rev: v6.0.0
|
| 22 |
hooks:
|
|
|
|
| 1 |
repos:
|
| 2 |
+
# ============================================
|
| 3 |
+
# BACKEND HOOKS (Python)
|
| 4 |
+
# ============================================
|
| 5 |
- repo: https://github.com/astral-sh/ruff-pre-commit
|
| 6 |
rev: v0.14.8
|
| 7 |
hooks:
|
|
|
|
| 20 |
# Exclude auto-generated Gradio custom component files
|
| 21 |
exclude: ^packages/niivueviewer/
|
| 22 |
|
| 23 |
+
# ============================================
|
| 24 |
+
# FRONTEND HOOKS (TypeScript/React)
|
| 25 |
+
# ============================================
|
| 26 |
+
- repo: local
|
| 27 |
+
hooks:
|
| 28 |
+
- id: frontend-lint
|
| 29 |
+
name: frontend-lint (eslint)
|
| 30 |
+
entry: bash -c 'cd frontend && npm run lint'
|
| 31 |
+
language: system
|
| 32 |
+
files: ^frontend/.*\.(ts|tsx|js|jsx)$
|
| 33 |
+
pass_filenames: false
|
| 34 |
+
|
| 35 |
+
- id: frontend-typecheck
|
| 36 |
+
name: frontend-typecheck (tsc)
|
| 37 |
+
entry: bash -c 'cd frontend && npx tsc --noEmit'
|
| 38 |
+
language: system
|
| 39 |
+
files: ^frontend/.*\.(ts|tsx)$
|
| 40 |
+
pass_filenames: false
|
| 41 |
+
|
| 42 |
+
- id: frontend-test
|
| 43 |
+
name: frontend-test (vitest coverage)
|
| 44 |
+
entry: bash -c 'cd frontend && npm run test:coverage'
|
| 45 |
+
language: system
|
| 46 |
+
files: ^frontend/.*\.(ts|tsx)$
|
| 47 |
+
pass_filenames: false
|
| 48 |
+
stages: [pre-push] # Run on push only (tests are slow)
|
| 49 |
+
|
| 50 |
+
# ============================================
|
| 51 |
+
# GENERAL HOOKS
|
| 52 |
+
# ============================================
|
| 53 |
- repo: https://github.com/pre-commit/pre-commit-hooks
|
| 54 |
rev: v6.0.0
|
| 55 |
hooks:
|
frontend/src/components/CaseSelector.tsx
CHANGED
|
@@ -1,4 +1,4 @@
|
|
| 1 |
-
import { useEffect, useState
|
| 2 |
import { apiClient, ApiError } from "../api/client";
|
| 3 |
|
| 4 |
// Cold start retry configuration (matches useSegmentation.ts)
|
|
@@ -21,60 +21,70 @@ export function CaseSelector({
|
|
| 21 |
const [retryCount, setRetryCount] = useState(0);
|
| 22 |
const [isWakingUp, setIsWakingUp] = useState(false);
|
| 23 |
|
| 24 |
-
|
| 25 |
-
|
|
|
|
|
|
|
|
|
|
| 26 |
|
| 27 |
-
|
| 28 |
-
|
| 29 |
-
const data = await apiClient.getCases(signal);
|
| 30 |
-
setCases(data.cases);
|
| 31 |
-
setIsWakingUp(false);
|
| 32 |
-
setRetryCount(0);
|
| 33 |
-
setIsLoading(false);
|
| 34 |
-
return; // Success
|
| 35 |
-
} catch (err) {
|
| 36 |
-
if (err instanceof Error && err.name === "AbortError") return;
|
| 37 |
|
| 38 |
-
|
| 39 |
-
|
| 40 |
-
|
| 41 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 42 |
|
| 43 |
-
|
| 44 |
-
|
| 45 |
-
|
| 46 |
-
|
| 47 |
-
setIsWakingUp(true);
|
| 48 |
|
| 49 |
-
//
|
| 50 |
-
|
| 51 |
-
|
| 52 |
-
|
| 53 |
-
|
| 54 |
-
await new Promise((resolve) => setTimeout(resolve, delay));
|
| 55 |
-
continue;
|
| 56 |
-
}
|
| 57 |
|
| 58 |
-
|
| 59 |
-
|
| 60 |
-
|
| 61 |
-
|
| 62 |
-
|
| 63 |
-
|
| 64 |
-
|
| 65 |
-
|
| 66 |
-
|
| 67 |
-
|
| 68 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 69 |
}
|
| 70 |
}
|
| 71 |
-
}, []);
|
| 72 |
|
| 73 |
-
|
| 74 |
-
|
| 75 |
-
|
| 76 |
-
|
| 77 |
-
|
|
|
|
|
|
|
| 78 |
|
| 79 |
if (isLoading) {
|
| 80 |
return (
|
|
|
|
| 1 |
+
import { useEffect, useState } from "react";
|
| 2 |
import { apiClient, ApiError } from "../api/client";
|
| 3 |
|
| 4 |
// Cold start retry configuration (matches useSegmentation.ts)
|
|
|
|
| 21 |
const [retryCount, setRetryCount] = useState(0);
|
| 22 |
const [isWakingUp, setIsWakingUp] = useState(false);
|
| 23 |
|
| 24 |
+
// Fetch cases on mount with cold-start retry logic
|
| 25 |
+
// Using inline async function pattern recommended by React docs for data fetching
|
| 26 |
+
useEffect(() => {
|
| 27 |
+
let isActive = true;
|
| 28 |
+
const abortController = new AbortController();
|
| 29 |
|
| 30 |
+
async function fetchCases() {
|
| 31 |
+
let attempts = 0;
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 32 |
|
| 33 |
+
while (attempts <= MAX_COLD_START_RETRIES && isActive) {
|
| 34 |
+
try {
|
| 35 |
+
const data = await apiClient.getCases(abortController.signal);
|
| 36 |
+
if (!isActive) return;
|
| 37 |
+
setCases(data.cases);
|
| 38 |
+
setIsWakingUp(false);
|
| 39 |
+
setRetryCount(0);
|
| 40 |
+
setIsLoading(false);
|
| 41 |
+
return; // Success
|
| 42 |
+
} catch (err) {
|
| 43 |
+
if (!isActive) return;
|
| 44 |
+
if (err instanceof Error && err.name === "AbortError") return;
|
| 45 |
|
| 46 |
+
const is503 = err instanceof ApiError && err.status === 503;
|
| 47 |
+
const isNetworkError =
|
| 48 |
+
err instanceof TypeError &&
|
| 49 |
+
err.message.toLowerCase().includes("fetch");
|
|
|
|
| 50 |
|
| 51 |
+
// Retry on cold start (503) or network errors
|
| 52 |
+
if ((is503 || isNetworkError) && attempts < MAX_COLD_START_RETRIES) {
|
| 53 |
+
attempts++;
|
| 54 |
+
setRetryCount(attempts);
|
| 55 |
+
setIsWakingUp(true);
|
|
|
|
|
|
|
|
|
|
| 56 |
|
| 57 |
+
// Exponential backoff
|
| 58 |
+
const delay = Math.min(
|
| 59 |
+
INITIAL_RETRY_DELAY * Math.pow(2, attempts - 1),
|
| 60 |
+
MAX_RETRY_DELAY,
|
| 61 |
+
);
|
| 62 |
+
await new Promise((resolve) => setTimeout(resolve, delay));
|
| 63 |
+
continue;
|
| 64 |
+
}
|
| 65 |
+
|
| 66 |
+
// Max retries exceeded or non-retryable error
|
| 67 |
+
const message =
|
| 68 |
+
is503 || isNetworkError
|
| 69 |
+
? "Backend failed to wake up. Please refresh the page."
|
| 70 |
+
: err instanceof Error
|
| 71 |
+
? err.message
|
| 72 |
+
: "Unknown error";
|
| 73 |
+
setError(`Failed to load cases: ${message}`);
|
| 74 |
+
setIsWakingUp(false);
|
| 75 |
+
setIsLoading(false);
|
| 76 |
+
return;
|
| 77 |
+
}
|
| 78 |
}
|
| 79 |
}
|
|
|
|
| 80 |
|
| 81 |
+
fetchCases();
|
| 82 |
+
|
| 83 |
+
return () => {
|
| 84 |
+
isActive = false;
|
| 85 |
+
abortController.abort();
|
| 86 |
+
};
|
| 87 |
+
}, []);
|
| 88 |
|
| 89 |
if (isLoading) {
|
| 90 |
return (
|
frontend/src/components/__tests__/CaseSelector.test.tsx
CHANGED
|
@@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
|
|
| 2 |
import { render, screen, waitFor } from "@testing-library/react";
|
| 3 |
import userEvent from "@testing-library/user-event";
|
| 4 |
import { server } from "../../mocks/server";
|
| 5 |
-
import { errorHandlers } from "../../mocks/handlers";
|
| 6 |
import { CaseSelector } from "../CaseSelector";
|
| 7 |
|
| 8 |
describe("CaseSelector", () => {
|
|
@@ -10,6 +10,7 @@ describe("CaseSelector", () => {
|
|
| 10 |
|
| 11 |
beforeEach(() => {
|
| 12 |
mockOnSelectCase.mockClear();
|
|
|
|
| 13 |
});
|
| 14 |
|
| 15 |
it("shows loading state initially", () => {
|
|
@@ -117,4 +118,32 @@ describe("CaseSelector", () => {
|
|
| 117 |
const container = screen.getByRole("combobox").closest("div");
|
| 118 |
expect(container).toHaveClass("bg-gray-800");
|
| 119 |
});
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 120 |
});
|
|
|
|
| 2 |
import { render, screen, waitFor } from "@testing-library/react";
|
| 3 |
import userEvent from "@testing-library/user-event";
|
| 4 |
import { server } from "../../mocks/server";
|
| 5 |
+
import { errorHandlers, resetCasesAttempts } from "../../mocks/handlers";
|
| 6 |
import { CaseSelector } from "../CaseSelector";
|
| 7 |
|
| 8 |
describe("CaseSelector", () => {
|
|
|
|
| 10 |
|
| 11 |
beforeEach(() => {
|
| 12 |
mockOnSelectCase.mockClear();
|
| 13 |
+
resetCasesAttempts();
|
| 14 |
});
|
| 15 |
|
| 16 |
it("shows loading state initially", () => {
|
|
|
|
| 118 |
const container = screen.getByRole("combobox").closest("div");
|
| 119 |
expect(container).toHaveClass("bg-gray-800");
|
| 120 |
});
|
| 121 |
+
|
| 122 |
+
it("retries on 503 cold-start and succeeds", async () => {
|
| 123 |
+
server.use(errorHandlers.casesColdStart);
|
| 124 |
+
|
| 125 |
+
render(
|
| 126 |
+
<CaseSelector selectedCase={null} onSelectCase={mockOnSelectCase} />,
|
| 127 |
+
);
|
| 128 |
+
|
| 129 |
+
// Should show waking up message during retry
|
| 130 |
+
await waitFor(
|
| 131 |
+
() => {
|
| 132 |
+
expect(screen.getByText(/waking up/i)).toBeInTheDocument();
|
| 133 |
+
},
|
| 134 |
+
{ timeout: 3000 },
|
| 135 |
+
);
|
| 136 |
+
|
| 137 |
+
// Should eventually succeed and show cases
|
| 138 |
+
await waitFor(
|
| 139 |
+
() => {
|
| 140 |
+
expect(screen.getByRole("combobox")).toBeInTheDocument();
|
| 141 |
+
},
|
| 142 |
+
{ timeout: 5000 },
|
| 143 |
+
);
|
| 144 |
+
|
| 145 |
+
expect(
|
| 146 |
+
screen.getByRole("option", { name: /sub-stroke0001/i }),
|
| 147 |
+
).toBeInTheDocument();
|
| 148 |
+
});
|
| 149 |
});
|
frontend/src/mocks/handlers.ts
CHANGED
|
@@ -173,6 +173,14 @@ export const handlers = [
|
|
| 173 |
}),
|
| 174 |
];
|
| 175 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 176 |
// Error handlers for testing error states
|
| 177 |
export const errorHandlers = {
|
| 178 |
casesServerError: http.get(`${API_BASE}/api/cases`, () => {
|
|
@@ -186,6 +194,21 @@ export const errorHandlers = {
|
|
| 186 |
return HttpResponse.error();
|
| 187 |
}),
|
| 188 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 189 |
segmentCreateError: http.post(`${API_BASE}/api/segment`, () => {
|
| 190 |
return HttpResponse.json(
|
| 191 |
{ detail: "Failed to create job: case not found" },
|
|
|
|
| 173 |
}),
|
| 174 |
];
|
| 175 |
|
| 176 |
+
// Track retry attempts for cold-start testing
|
| 177 |
+
let casesAttempts = 0;
|
| 178 |
+
|
| 179 |
+
/** Reset the cases attempt counter (call in test beforeEach) */
|
| 180 |
+
export function resetCasesAttempts(): void {
|
| 181 |
+
casesAttempts = 0;
|
| 182 |
+
}
|
| 183 |
+
|
| 184 |
// Error handlers for testing error states
|
| 185 |
export const errorHandlers = {
|
| 186 |
casesServerError: http.get(`${API_BASE}/api/cases`, () => {
|
|
|
|
| 194 |
return HttpResponse.error();
|
| 195 |
}),
|
| 196 |
|
| 197 |
+
// 503 on first attempt, success on retry (tests cold-start retry)
|
| 198 |
+
casesColdStart: http.get(`${API_BASE}/api/cases`, async () => {
|
| 199 |
+
casesAttempts++;
|
| 200 |
+
if (casesAttempts === 1) {
|
| 201 |
+
return HttpResponse.json(
|
| 202 |
+
{ detail: "Service Unavailable" },
|
| 203 |
+
{ status: 503 },
|
| 204 |
+
);
|
| 205 |
+
}
|
| 206 |
+
// Succeed on retry
|
| 207 |
+
return HttpResponse.json({
|
| 208 |
+
cases: ["sub-stroke0001", "sub-stroke0002", "sub-stroke0003"],
|
| 209 |
+
});
|
| 210 |
+
}),
|
| 211 |
+
|
| 212 |
segmentCreateError: http.post(`${API_BASE}/api/segment`, () => {
|
| 213 |
return HttpResponse.json(
|
| 214 |
{ detail: "Failed to create job: case not found" },
|