refactor: modularize match logic and UI components (#85)#222
refactor: modularize match logic and UI components (#85)#222BenedictHomuth wants to merge 3 commits intomainfrom
Conversation
| </Button> | ||
| </Tooltip> | ||
| </SimpleGrid> | ||
| <Button |
There was a problem hiding this comment.
I noticed a layout shift introduced by this PR.
Before:
After:
The “Next player” button has moved below the double/triple controls in the new layout. While testing, I accidentally skipped player turns because of this change 😄
There was a problem hiding this comment.
Pull request overview
This PR refactors the darts match implementation by extracting core match state transitions into a dedicated reducer/utils layer and splitting the playing UI into smaller, focused components, while adding a Vitest + Testing Library setup and new tests.
Changes:
- Introduces a pure
gameReducer+isWinningThrowutility for match state transitions and win detection. - Modularizes the match UI into
MatchPlayerCard,MatchControls, andMatchRoundInfo. - Adds Vitest React/jsdom support and introduces new hook + integration tests.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
vitest.config.js |
Adds React plugin, jsdom environment, and additional path aliases for tests. |
package.json |
Adds React Testing Library, jsdom, and Vite React plugin as devDependencies. |
package-lock.json |
Locks newly added devDependencies. |
renderer/hooks/useDartGame/reducer.ts |
New pure reducer for match state transitions (throws, turn switching, leg/set/match win). |
renderer/hooks/useDartGame/utils.ts |
Extracts isWinningThrow into a standalone utility. |
renderer/hooks/useDartGame.ts |
Uses extracted reducer (hook now orchestrates effects/timing). |
renderer/pages/[locale]/match/playing.tsx |
Replaces monolithic UI with modular match components. |
renderer/components/match/MatchPlayerCard.tsx |
New component encapsulating player display + stats/progress UI. |
renderer/components/match/MatchControls.tsx |
New component encapsulating dartboard inputs and turn controls. |
renderer/components/match/MatchRoundInfo.tsx |
New component showing round totals/throws and checkout hints. |
renderer/tests/hooks/useDartGame.test.ts |
Updates reducer import path after extraction. |
renderer/tests/hooks/useDartGamePersistence.test.ts |
Adds persistence/hydration-focused tests for the hook. |
renderer/tests/match/integration/HappyPath.test.tsx |
Adds a high-level integration test wiring Playing UI interactions to hook actions/navigation. |
| double: boolean; | ||
| triple: boolean; | ||
| }; | ||
| matchStatus: string; | ||
| matchRoundLength: number; |
There was a problem hiding this comment.
matchStatus is typed as string, which loses the MatchStatus union and diverges from existing usage (e.g., renderer/components/content/BadgeMatchStatus.tsx:6 uses MatchStatus). Prefer typing this prop as MatchStatus (from types/match) to keep compile-time guarantees (and avoid accidental typos like "finised").
| import { describe, expect, it, vi, beforeEach, type Mock } from "vitest"; | ||
| import { render, screen, fireEvent, waitFor } from "@testing-library/react"; | ||
| import { MantineProvider } from "@mantine/core"; | ||
| import PlayingPage from "@/pages/[locale]/match/playing"; | ||
|
|
||
| // Mock window.matchMedia which is required by Mantine | ||
| Object.defineProperty(window, "matchMedia", { | ||
| writable: true, | ||
| value: vi.fn().mockImplementation((query: string) => ({ | ||
| matches: false, | ||
| media: query, | ||
| onchange: null, | ||
| addListener: vi.fn(), // deprecated | ||
| removeListener: vi.fn(), // deprecated | ||
| addEventListener: vi.fn(), | ||
| removeEventListener: vi.fn(), | ||
| dispatchEvent: vi.fn(), | ||
| })), | ||
| }); |
There was a problem hiding this comment.
The matchMedia/localStorage stubs are declared after static imports. Because ESM imports are hoisted and evaluated before module body code runs, these stubs may not be in place when Mantine/PlayingPage modules are evaluated, leading to brittle tests. Consider moving these stubs to a Vitest setupFiles entry (or using a dynamic import of PlayingPage after stubbing globals).
| it("should complete a full 101-Single match successfully", async () => { | ||
| // 1. Setup initial state for a 101 game | ||
| const gameState = { | ||
| state: { | ||
| isHydrated: true, | ||
| players: [ | ||
| { | ||
| uuid: "p1", | ||
| username: "Alice", | ||
| name: { firstName: "Alice", lastName: "" }, | ||
| scoreLeft: 101, | ||
| rounds: [], | ||
| isWinner: false, | ||
| legsWon: 0, | ||
| setsWon: 0, | ||
| color: "blue", | ||
| statistics: { | ||
| average: 0, | ||
| playedMatches: 0, | ||
| playedTrainings: 0, | ||
| thrownDarts: 0, | ||
| thrownOneHundredAndEighty: 0, | ||
| }, | ||
| }, | ||
| ], | ||
| currentPlayerIndex: 0, | ||
| currentLegStartingPlayerIndex: 0, | ||
| matchRound: [], | ||
| multiplier: { double: false, triple: false }, | ||
| matchStatus: "started", | ||
| initialScore: 101, | ||
| matchCheckout: "Single", | ||
| uuid: "match-123", | ||
| legs: 1, | ||
| sets: 1, | ||
| appVersion: "1.0.0", | ||
| createdAt: Date.now(), | ||
| }, | ||
| actions: { | ||
| throwDart: vi.fn(), | ||
| nextTurn: vi.fn(), | ||
| undoThrow: vi.fn(), | ||
| toggleMultiplier: vi.fn(), | ||
| abortMatch: vi.fn(), | ||
| }, | ||
| }; | ||
|
|
||
| (useDartGame as Mock).mockReturnValue(gameState); | ||
|
|
||
| const { rerender } = render( | ||
| <MantineProvider> | ||
| <PlayingPage /> | ||
| </MantineProvider>, | ||
| ); | ||
|
|
||
| // Verify initial render - Use getAllByText because username and firstName might both be Alice | ||
| expect(screen.getAllByText("Alice").length).toBeGreaterThan(0); | ||
| expect(screen.getByText("101")).toBeDefined(); | ||
|
|
||
| // 2. Simulate Alice throwing T20, T10, 11 (Sum: 60 + 30 + 11 = 101) | ||
| // In this integration test, we verify that clicking UI buttons calls the correct actions | ||
|
|
||
| // Throw T20 | ||
| fireEvent.click(screen.getByText("match:multipliers.triple")); | ||
| expect(gameState.actions.toggleMultiplier).toHaveBeenCalledWith("triple"); | ||
|
|
||
| fireEvent.click(screen.getByText("20")); | ||
| expect(gameState.actions.throwDart).toHaveBeenCalledWith(20); | ||
|
|
||
| // Update state to reflect the win (Simulating what the hook/reducer would do) | ||
| gameState.state.players[0].scoreLeft = 0; | ||
| gameState.state.players[0].isWinner = true; | ||
| gameState.state.matchStatus = "finished"; | ||
| (useDartGame as Mock).mockReturnValue(gameState); |
There was a problem hiding this comment.
Test name/comments say this simulates a full “101 … T20, T10, 11” checkout, but the test currently only clicks the triple toggle and “20” before manually forcing matchStatus to finished. Either update the description to match what’s actually asserted (UI wiring), or extend the test to execute the full sequence it claims.
| // Mock Mantine's useSessionStorage | ||
| const mockSetSessionStorage = vi.fn(); | ||
| const mockUseSessionStorage = vi.fn(); | ||
|
|
||
| vi.mock("@mantine/hooks", async (importOriginal) => { | ||
| const actual = await importOriginal<typeof mantineHooks>(); | ||
| return { | ||
| ...actual, | ||
| useSessionStorage: (args: unknown) => | ||
| mockUseSessionStorage(args) as [unknown, (value: unknown) => void], | ||
| }; | ||
| }); | ||
|
|
||
| // Mock use-elapsed-time | ||
| vi.mock("use-elapsed-time", () => ({ | ||
| useElapsedTime: vi.fn(() => ({ | ||
| elapsedTime: 10, | ||
| reset: vi.fn(), | ||
| })), | ||
| })); | ||
|
|
||
| describe("useDartGame Persistence and Hydration", () => { | ||
| it("should initialize with default state when no persisted data exists", () => { | ||
| mockUseSessionStorage.mockReturnValue([undefined, mockSetSessionStorage]); | ||
| const { result } = renderHook(() => useDartGame()); | ||
|
|
||
| expect(result.current.state.isHydrated).toBe(false); | ||
| expect(result.current.state.matchStatus).toBe("undefined"); |
There was a problem hiding this comment.
The test doesn’t reset mockSetSessionStorage/mockUseSessionStorage between cases, so assertions like toHaveBeenCalled() can pass due to calls from earlier tests and make lastCall harder to reason about. Add a beforeEach(() => { vi.clearAllMocks(); ... }) (or reset these specific mocks) to keep each test isolated.
|
|
||
| // 2. Perform an action that changes state | ||
| act(() => { | ||
| result.current.actions.throwDart(20); | ||
| }); | ||
|
|
||
| // 3. Check if persistence setter was called | ||
| // Persistence might be called on first render too, so we check the latest call | ||
| expect(mockSetSessionStorage).toHaveBeenCalled(); | ||
|
|
||
| act(() => { | ||
| result.current.actions.nextTurn(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
In this test, throwDart(20) changes matchRound only; if persistence is tied to players/turn transitions, setPersistedMatchData may not be invoked by this action. As written, expect(mockSetSessionStorage).toHaveBeenCalled() can be satisfied by hydration/initial persistence rather than the throw. Consider asserting call counts before/after the action (or persisting matchRound if the intent is to persist every dart).
| // 2. Perform an action that changes state | |
| act(() => { | |
| result.current.actions.throwDart(20); | |
| }); | |
| // 3. Check if persistence setter was called | |
| // Persistence might be called on first render too, so we check the latest call | |
| expect(mockSetSessionStorage).toHaveBeenCalled(); | |
| act(() => { | |
| result.current.actions.nextTurn(); | |
| }); | |
| const initialPersistCallCount = mockSetSessionStorage.mock.calls.length; | |
| // 2. Perform an action that updates round state only | |
| act(() => { | |
| result.current.actions.throwDart(20); | |
| }); | |
| const persistCallCountAfterThrow = mockSetSessionStorage.mock.calls.length; | |
| act(() => { | |
| result.current.actions.nextTurn(); | |
| }); | |
| // 3. Check if persistence setter was called because of the state transition | |
| expect(mockSetSessionStorage.mock.calls.length).toBeGreaterThan( | |
| persistCallCountAfterThrow | |
| ); | |
| expect(persistCallCountAfterThrow).toBeGreaterThanOrEqual( | |
| initialPersistCallCount | |
| ); |
|
|
||
| // 2. Check Bust | ||
| const newScoreLeft = currentPlayer.scoreLeft - totalRoundScore; | ||
| const bust = isBust(state.matchCheckout, newScoreLeft); |
There was a problem hiding this comment.
In NEXT_TURN, a player can end the round with newScoreLeft === 0 but isLegWinner === false (e.g., Double/Triple checkout where the last dart doesn’t satisfy the checkout requirement). In that case bust stays false (isBust only checks negative/<2/<3), so the reducer sets scoreLeft to 0 anyway, effectively allowing an invalid checkout. Treat newScoreLeft === 0 && !isLegWinner as a bust (or extend isBust/isWinningThrow handling) so the score reverts instead of reaching 0.
| const bust = isBust(state.matchCheckout, newScoreLeft); | |
| const bust = | |
| isBust(state.matchCheckout, newScoreLeft) || | |
| (newScoreLeft === 0 && !isLegWinner); |
This PR addresses Issue #85 by refactoring the core darts match implementation. The primary goal was to decompose the "God files" (playing.tsx and useDartGame.ts) into focused, maintainable modules while establishing a robust testing
foundation that ensures long-term stability without introducing production bloat.
🏗️ Architectural Changes
We have moved away from a monolithic structure to a modular, component-based architecture.
The match interface has been split into three specialized components:
Reasoning: Splitting these reduces the cognitive load for developers. If you need to fix a UI bug in the keypad, you no longer have to navigate through 400 lines of unrelated player-stat code.
🧪 Testing Architecture & Ergonomics
To ensure this refactor didn't break core features and to protect the project against future regressions, we have introduced a modern testing layer.
Why new packages were added:
The previous setup was only capable of testing pure mathematical functions. To build a "stable testbase," we needed to test how the app behaves in a browser-like environment. We added:
New Test Capabilities:
Developer Ergonomics: These tools provide immediate feedback. If a future change accidentally breaks the connection between a button and the game logic, these tests will catch it in seconds.
Note: These are strictly devDependencies
and have zero impact on the final application size.
🛠️ Maintenance & Quality
Verification Results