From bbfb044b5dca381ab920b64eb59ed9c46db00814 Mon Sep 17 00:00:00 2001 From: "Khoa (Revenovich) Tran Gia" Date: Fri, 17 Apr 2026 13:23:21 +0700 Subject: [PATCH] fix: validate per-step fields before advancing Create Server wizard - Add STEP_FIELDS constant mapping each step to its required fields - Extract trigger() from useForm and call it on Next click - Only advance to next step when trigger() returns true, blocking silent failures where invalid data could reach the Review step - Add CreateServerPage.test.tsx with 8 tests covering step navigation, validation blocking, happy path, and submit mutation - Update CLAUDE.md: mark /servers/new Complete, remove resolved bug - Mark implementation plan as completed --- .claude/plan/fix-create-server-submit.md | 146 +++++++++++++ CLAUDE.md | 6 +- .../src/__tests__/CreateServerPage.test.tsx | 197 ++++++++++++++++++ frontend/src/pages/CreateServerPage.tsx | 13 +- 4 files changed, 356 insertions(+), 6 deletions(-) create mode 100644 .claude/plan/fix-create-server-submit.md create mode 100644 frontend/src/__tests__/CreateServerPage.test.tsx diff --git a/.claude/plan/fix-create-server-submit.md b/.claude/plan/fix-create-server-submit.md new file mode 100644 index 0000000..40822b8 --- /dev/null +++ b/.claude/plan/fix-create-server-submit.md @@ -0,0 +1,146 @@ +# Implementation Plan: Fix Create Server Submit Button + +> **Status: IMPLEMENTED** — All 3 code changes applied, 8 tests added, 128/128 passing. +> Implemented: 2026-04-17 + +## Task Type +- [x] Frontend only + +## Problem Analysis + +**Root Cause** (`frontend/src/pages/CreateServerPage.tsx:266-273`): + +The "Next" button advances steps unconditionally: +```tsx +onClick={() => setStep(step + 1)} +``` + +Users can proceed through steps 0→1→2→3 with invalid data in steps 0–2. On step 3, `handleSubmit(onSubmit)` runs Zod validation across the whole schema — if any required field (name, exe_path, game_port, game_type) is empty or malformed, validation fails and `onSubmit` is **never called**. There is no error shown because RHF simply blocks submission silently when in an invalid state. + +The step 3 error block (lines 229-237) only shows `errors` that RHF knows about — but RHF only populates `errors` after a submit attempt, not after just navigating to a step. So the Review step also appears clean even when data is bad. + +## Technical Solution + +Extract `trigger` from `useForm` and call it per-step before advancing. +`trigger(fields)` runs Zod validation for the given fields only, populates `formState.errors`, +and returns `true` if all pass. Only advance to the next step when it returns `true`. + +### Per-step field mapping + +| Step | Label | Fields to validate | +|------|-------|-------------------| +| 0 | Game Type | `game_type` | +| 1 | Server Info | `name`, `exe_path`, `game_port`, `rcon_port` | +| 2 | Options | `max_restarts` | +| 3 | Review | (submit — no additional trigger needed) | + +## Implementation Steps + +### Step 1 — Extract `trigger` from `useForm` + +**File:** `frontend/src/pages/CreateServerPage.tsx:38-56` + +Change: +```tsx +const { + register, + handleSubmit, + watch, + setValue, + formState: { errors }, +} = useForm({ ... }); +``` + +To: +```tsx +const { + register, + handleSubmit, + watch, + setValue, + trigger, + formState: { errors }, +} = useForm({ ... }); +``` + +### Step 2 — Add per-step field mapping constant + +Add after `const STEPS = ["Game Type", "Server Info", "Options", "Review"];`: + +```tsx +const STEP_FIELDS: Array<(keyof CreateServerForm)[]> = [ + ["game_type"], // step 0 + ["name", "exe_path", "game_port", "rcon_port"], // step 1 + ["max_restarts"], // step 2 + [], // step 3 (review — submit handles it) +]; +``` + +### Step 3 — Replace Next button onClick with validated handler + +**File:** `frontend/src/pages/CreateServerPage.tsx:266-274` + +Change: +```tsx + +``` + +To: +```tsx + +``` + +### Step 4 — Add CreateServerPage unit tests + +**File:** `frontend/src/__tests__/CreateServerPage.test.tsx` (new file) + +Tests to cover: + +1. **Renders step 0 by default** — Game Type selector visible +2. **Next on step 0 advances to step 1** — server info fields visible +3. **Next on step 1 with empty name blocks advance** — stays on step 1, error message shown +4. **Next on step 1 with empty exe_path blocks advance** — stays on step 1, error shown +5. **Next on step 1 with valid data advances to step 2** — options fields visible +6. **Back button on step 1 returns to step 0** +7. **Full happy path reaches Review step** — review table visible +8. **Submit fires createServer mutation on valid data** + +Test setup: mock `useCreateServer`, `useGamesList`, `useAuthStore` (admin), `useUIStore`. + +## Key Files + +| File | Operation | Description | +|------|-----------|-------------| +| `frontend/src/pages/CreateServerPage.tsx:38-56` | Modify | Add `trigger` to useForm destructure | +| `frontend/src/pages/CreateServerPage.tsx:27` | Modify | Add `STEP_FIELDS` constant after `STEPS` | +| `frontend/src/pages/CreateServerPage.tsx:266-274` | Modify | Replace Next onClick with async trigger-guarded handler | +| `frontend/src/__tests__/CreateServerPage.test.tsx` | Create | Unit tests for wizard validation | + +## Risks and Mitigation + +| Risk | Mitigation | +|------|------------| +| `trigger` for `rcon_port` (nullable optional) might block valid empty input | `rcon_port` schema is `.nullable().optional()` so empty/null passes — no issue | +| `max_restarts` has `valueAsNumber` — NaN on empty input | Schema has `.min(0).max(20).optional()` with coerce; default is 3 so this won't be empty in practice | +| Async onClick on a `type="button"` | Safe — button is not `type="submit"`, no double-submit risk | + +## SESSION_ID +- CODEX_SESSION: N/A (codeagent-wrapper not available) +- GEMINI_SESSION: N/A (codeagent-wrapper not available) diff --git a/CLAUDE.md b/CLAUDE.md index 284e6c6..80d3916 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,13 +37,9 @@ All routers, services, repositories, game adapter system, WebSocket, background | `/login` | Complete | Zod + react-hook-form validation | | `/` | Complete | Dashboard with server grid | | `/servers/:id` | Complete | 7-tab detail page (overview, config, players, bans, missions, mods, logs) | -| `/servers/new` | Partial | 4-step wizard; **known bug: form validation issues cause silent failure on submit** | +| `/servers/new` | Complete | 4-step wizard with per-step validation via `trigger()` | | `/settings` | Complete | Password change + admin user management | -### Known Bugs (as of 2026-04-17) - -1. **Create Server silent failure**: The 4-step wizard's "Next" buttons don't validate before advancing steps, so users can reach step 3 with invalid data. `handleSubmit` then silently fails because validation errors prevent `onSubmit` from firing. Fix: validate on each "Next" click using `trigger()` from react-hook-form. - ### Frontend Type Mapping (API → Frontend) | API Resource | Frontend Type | Key Fields | diff --git a/frontend/src/__tests__/CreateServerPage.test.tsx b/frontend/src/__tests__/CreateServerPage.test.tsx new file mode 100644 index 0000000..3c77ab5 --- /dev/null +++ b/frontend/src/__tests__/CreateServerPage.test.tsx @@ -0,0 +1,197 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { MemoryRouter, Route, Routes } from "react-router-dom"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +import { CreateServerPage } from "@/pages/CreateServerPage"; +import { useCreateServer } from "@/hooks/useServers"; +import { useGamesList } from "@/hooks/useGames"; +import { useAuthStore } from "@/store/auth.store"; +import { useUIStore } from "@/store/ui.store"; + +vi.mock("@/hooks/useServers", () => ({ + useCreateServer: vi.fn(), +})); + +vi.mock("@/hooks/useGames", () => ({ + useGamesList: vi.fn(), +})); + +vi.mock("@/store/auth.store", () => ({ + useAuthStore: vi.fn(), +})); + +vi.mock("@/store/ui.store", () => ({ + useUIStore: vi.fn(), +})); + +const mockMutateAsync = vi.fn(); +const mockAddNotification = vi.fn(); + +function renderPage() { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + + return { + user: userEvent.setup(), + ...render( + + + + } /> + Dashboard} /> + Server Detail} /> + + + , + ), + }; +} + +describe("CreateServerPage", () => { + beforeEach(() => { + vi.mocked(useAuthStore).mockReturnValue("admin"); + vi.mocked(useUIStore).mockReturnValue(mockAddNotification); + vi.mocked(useGamesList).mockReturnValue({ data: undefined } as ReturnType); + vi.mocked(useCreateServer).mockReturnValue({ + mutateAsync: mockMutateAsync, + isPending: false, + } as unknown as ReturnType); + mockMutateAsync.mockReset(); + mockAddNotification.mockReset(); + }); + + it("renders step 0 (Game Type) by default", () => { + renderPage(); + expect(screen.getByText("Create Server")).toBeInTheDocument(); + expect(screen.getByRole("combobox")).toBeInTheDocument(); + expect(screen.queryByPlaceholderText("My Arma Server")).not.toBeInTheDocument(); + }); + + it("Next on step 0 advances to step 1", async () => { + const { user } = renderPage(); + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => { + expect(screen.getByPlaceholderText("My Arma Server")).toBeInTheDocument(); + }); + }); + + it("Next on step 1 with empty name blocks advance and shows error", async () => { + const { user } = renderPage(); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => expect(screen.getByPlaceholderText("My Arma Server")).toBeInTheDocument()); + + await user.click(screen.getByRole("button", { name: /next/i })); + + await waitFor(() => { + expect(screen.getByText("Server name is required")).toBeInTheDocument(); + }); + expect(screen.getByPlaceholderText("My Arma Server")).toBeInTheDocument(); + }); + + it("Next on step 1 with empty exe_path blocks advance and shows error", async () => { + const { user } = renderPage(); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => expect(screen.getByPlaceholderText("My Arma Server")).toBeInTheDocument()); + + await user.type(screen.getByPlaceholderText("My Arma Server"), "Test Server"); + await user.click(screen.getByRole("button", { name: /next/i })); + + await waitFor(() => { + expect(screen.getByText("Executable path is required")).toBeInTheDocument(); + }); + expect(screen.getByPlaceholderText("My Arma Server")).toBeInTheDocument(); + }); + + it("Next on step 1 with valid data advances to step 2", async () => { + const { user } = renderPage(); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => expect(screen.getByPlaceholderText("My Arma Server")).toBeInTheDocument()); + + await user.type(screen.getByPlaceholderText("My Arma Server"), "My Server"); + await user.type( + screen.getByPlaceholderText("D:/Arma3Server/arma3server_x64.exe"), + "C:/server/arma3.exe", + ); + + await user.click(screen.getByRole("button", { name: /next/i })); + + await waitFor(() => { + expect(screen.getByLabelText(/auto-restart on crash/i)).toBeInTheDocument(); + }); + }); + + it("Back button on step 1 returns to step 0", async () => { + const { user } = renderPage(); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => expect(screen.getByPlaceholderText("My Arma Server")).toBeInTheDocument()); + + await user.click(screen.getByRole("button", { name: /back/i })); + + await waitFor(() => { + expect(screen.getByRole("combobox")).toBeInTheDocument(); + }); + expect(screen.queryByPlaceholderText("My Arma Server")).not.toBeInTheDocument(); + }); + + it("full happy path reaches Review step", async () => { + const { user } = renderPage(); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => expect(screen.getByPlaceholderText("My Arma Server")).toBeInTheDocument()); + + await user.type(screen.getByPlaceholderText("My Arma Server"), "My Server"); + await user.type( + screen.getByPlaceholderText("D:/Arma3Server/arma3server_x64.exe"), + "C:/server/arma3.exe", + ); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => expect(screen.getByLabelText(/auto-restart on crash/i)).toBeInTheDocument()); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => { + expect(screen.getByText("Review Configuration")).toBeInTheDocument(); + }); + expect(screen.getByRole("button", { name: /create server/i })).toBeInTheDocument(); + }); + + it("submit fires createServer mutation on valid data", async () => { + mockMutateAsync.mockResolvedValueOnce({ data: { id: 42 } }); + + const { user } = renderPage(); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => expect(screen.getByPlaceholderText("My Arma Server")).toBeInTheDocument()); + + await user.type(screen.getByPlaceholderText("My Arma Server"), "My Server"); + await user.type( + screen.getByPlaceholderText("D:/Arma3Server/arma3server_x64.exe"), + "C:/server/arma3.exe", + ); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => expect(screen.getByLabelText(/auto-restart on crash/i)).toBeInTheDocument()); + + await user.click(screen.getByRole("button", { name: /next/i })); + await waitFor(() => expect(screen.getByText("Review Configuration")).toBeInTheDocument()); + + await user.click(screen.getByRole("button", { name: /create server/i })); + + await waitFor(() => { + expect(mockMutateAsync).toHaveBeenCalledWith( + expect.objectContaining({ + name: "My Server", + exe_path: "C:/server/arma3.exe", + game_type: "arma3", + }), + ); + }); + }); +}); diff --git a/frontend/src/pages/CreateServerPage.tsx b/frontend/src/pages/CreateServerPage.tsx index 87252e7..a9038f1 100644 --- a/frontend/src/pages/CreateServerPage.tsx +++ b/frontend/src/pages/CreateServerPage.tsx @@ -26,6 +26,13 @@ type CreateServerForm = z.infer; const STEPS = ["Game Type", "Server Info", "Options", "Review"]; +const STEP_FIELDS: Array<(keyof CreateServerForm)[]> = [ + ["game_type"], + ["name", "exe_path", "game_port", "rcon_port"], + ["max_restarts"], + [], +]; + export function CreateServerPage() { const navigate = useNavigate(); const isAdmin = useAuthStore((s) => s.user?.role === "admin"); @@ -40,6 +47,7 @@ export function CreateServerPage() { handleSubmit, watch, setValue, + trigger, formState: { errors }, } = useForm({ resolver: zodResolver(createServerSchema), @@ -266,7 +274,10 @@ export function CreateServerPage() { {step < STEPS.length - 1 ? (