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
This commit is contained in:
146
.claude/plan/fix-create-server-submit.md
Normal file
146
.claude/plan/fix-create-server-submit.md
Normal file
@@ -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<CreateServerForm>({ ... });
|
||||||
|
```
|
||||||
|
|
||||||
|
To:
|
||||||
|
```tsx
|
||||||
|
const {
|
||||||
|
register,
|
||||||
|
handleSubmit,
|
||||||
|
watch,
|
||||||
|
setValue,
|
||||||
|
trigger,
|
||||||
|
formState: { errors },
|
||||||
|
} = useForm<CreateServerForm>({ ... });
|
||||||
|
```
|
||||||
|
|
||||||
|
### 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
|
||||||
|
<button
|
||||||
|
type="button"
|
||||||
|
onClick={() => setStep(step + 1)}
|
||||||
|
className="btn-primary flex items-center gap-1.5"
|
||||||
|
>
|
||||||
|
Next
|
||||||
|
<ChevronRight size={16} />
|
||||||
|
</button>
|
||||||
|
```
|
||||||
|
|
||||||
|
To:
|
||||||
|
```tsx
|
||||||
|
<button
|
||||||
|
type="button"
|
||||||
|
onClick={async () => {
|
||||||
|
const valid = await trigger(STEP_FIELDS[step]);
|
||||||
|
if (valid) setStep(step + 1);
|
||||||
|
}}
|
||||||
|
className="btn-primary flex items-center gap-1.5"
|
||||||
|
>
|
||||||
|
Next
|
||||||
|
<ChevronRight size={16} />
|
||||||
|
</button>
|
||||||
|
```
|
||||||
|
|
||||||
|
### 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)
|
||||||
@@ -37,13 +37,9 @@ All routers, services, repositories, game adapter system, WebSocket, background
|
|||||||
| `/login` | Complete | Zod + react-hook-form validation |
|
| `/login` | Complete | Zod + react-hook-form validation |
|
||||||
| `/` | Complete | Dashboard with server grid |
|
| `/` | Complete | Dashboard with server grid |
|
||||||
| `/servers/:id` | Complete | 7-tab detail page (overview, config, players, bans, missions, mods, logs) |
|
| `/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 |
|
| `/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)
|
### Frontend Type Mapping (API → Frontend)
|
||||||
|
|
||||||
| API Resource | Frontend Type | Key Fields |
|
| API Resource | Frontend Type | Key Fields |
|
||||||
|
|||||||
197
frontend/src/__tests__/CreateServerPage.test.tsx
Normal file
197
frontend/src/__tests__/CreateServerPage.test.tsx
Normal file
@@ -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(
|
||||||
|
<QueryClientProvider client={queryClient}>
|
||||||
|
<MemoryRouter initialEntries={["/servers/new"]}>
|
||||||
|
<Routes>
|
||||||
|
<Route path="/servers/new" element={<CreateServerPage />} />
|
||||||
|
<Route path="/" element={<div>Dashboard</div>} />
|
||||||
|
<Route path="/servers/:id" element={<div>Server Detail</div>} />
|
||||||
|
</Routes>
|
||||||
|
</MemoryRouter>
|
||||||
|
</QueryClientProvider>,
|
||||||
|
),
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
describe("CreateServerPage", () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
vi.mocked(useAuthStore).mockReturnValue("admin");
|
||||||
|
vi.mocked(useUIStore).mockReturnValue(mockAddNotification);
|
||||||
|
vi.mocked(useGamesList).mockReturnValue({ data: undefined } as ReturnType<typeof useGamesList>);
|
||||||
|
vi.mocked(useCreateServer).mockReturnValue({
|
||||||
|
mutateAsync: mockMutateAsync,
|
||||||
|
isPending: false,
|
||||||
|
} as unknown as ReturnType<typeof useCreateServer>);
|
||||||
|
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",
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -26,6 +26,13 @@ type CreateServerForm = z.infer<typeof createServerSchema>;
|
|||||||
|
|
||||||
const STEPS = ["Game Type", "Server Info", "Options", "Review"];
|
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() {
|
export function CreateServerPage() {
|
||||||
const navigate = useNavigate();
|
const navigate = useNavigate();
|
||||||
const isAdmin = useAuthStore((s) => s.user?.role === "admin");
|
const isAdmin = useAuthStore((s) => s.user?.role === "admin");
|
||||||
@@ -40,6 +47,7 @@ export function CreateServerPage() {
|
|||||||
handleSubmit,
|
handleSubmit,
|
||||||
watch,
|
watch,
|
||||||
setValue,
|
setValue,
|
||||||
|
trigger,
|
||||||
formState: { errors },
|
formState: { errors },
|
||||||
} = useForm<CreateServerForm>({
|
} = useForm<CreateServerForm>({
|
||||||
resolver: zodResolver(createServerSchema),
|
resolver: zodResolver(createServerSchema),
|
||||||
@@ -266,7 +274,10 @@ export function CreateServerPage() {
|
|||||||
{step < STEPS.length - 1 ? (
|
{step < STEPS.length - 1 ? (
|
||||||
<button
|
<button
|
||||||
type="button"
|
type="button"
|
||||||
onClick={() => setStep(step + 1)}
|
onClick={async () => {
|
||||||
|
const valid = await trigger(STEP_FIELDS[step]);
|
||||||
|
if (valid) setStep(step + 1);
|
||||||
|
}}
|
||||||
className="btn-primary flex items-center gap-1.5"
|
className="btn-primary flex items-center gap-1.5"
|
||||||
>
|
>
|
||||||
Next
|
Next
|
||||||
|
|||||||
Reference in New Issue
Block a user