test(e2e): add testids to Board/Cell + Playwright probe - #11
Conversation
- Rewrite MinimaxAI evaluation to support 3/4 player modes: - evaluateZOCDistance() now calculates for all players dynamically - evaluateTerritoryPotential() compares current player vs all opponents - actionHeuristic() considers all opponents - Zobrist hashing supports Y and G player stones - Move ordering added at all minimax levels - Improve PeerJS connection reliability: - Add 8 STUN servers for better NAT traversal - Add retry mechanism with exponential backoff (3 retries) - Increase join timeout to 30 seconds - Add connection status tracking - Improve multiplayer error handling: - Add ERROR_MESSAGES with i18n keys and suggestions - Add new error types: PEER_UNAVAILABLE, NETWORK_ERROR, TIMEOUT - Update all 6 locale files with descriptive error messages - Add comprehensive test coverage: - minimax-ai.multiplayer.test.ts (8 tests) - multiplayerStore.error.test.ts (8 tests) - multiplayerStore.ice.test.ts (5 tests) - ai.action.test.ts (8 tests) - ai.multiplayer.test.ts (8 tests) All 65 tests passing, build successful.
…action application
fix(store): applyActionSequence actor detection & test cleanup
…ean handlers on cancel
…sure no late AI replies applied
…sure no late AI replies applied
Add unit tests verifying TurnManager correctly handles late AI replies: - turnmanager.race.test.ts: Tests delayed agent responses are ignored after timeout - turnmanager.worker.test.ts: Tests worker simulation with FakeWorker for timing scenarios Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Verifies MinimaxAI.getBestMove correctly selects from getLegalActions and handles claimed territory filtering. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…ate AI replies Prevents race condition where delayed AI worker responses could be applied after timeout: - TurnManager: Pass requestId to agent.getAction(); verify returned requestId matches - AIWorker: Include requestId in postMessage response - PlayerAgent: Extend interface to accept optional requestId parameter - RandomAgent/MinimaxAgent: Add cancel flag, ignore late messages, implement cancel() - HumanAgent: Conform to updated PlayerAgent interface This ensures late AI replies (arriving after timeout/cancellation) are discarded. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Summary of ChangesHello @schaoss, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the game's capabilities by introducing online multiplayer and support for up to four players. It includes a complete overhaul of the game's core logic to handle multi-player dynamics, robust AI agent management, and a new wall-breaking mechanic. Additionally, it lays the groundwork for more reliable end-to-end testing by adding specific test identifiers to key UI components. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b005f410bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // we must ignore it. Use try/catch to handle agent promise rejection. | ||
| let action: PlayerAction | undefined | ||
| try { | ||
| const result = await Promise.race([agent.getAction(this.getGameState(), requestId), timeoutPromise]) |
There was a problem hiding this comment.
Here agent.getAction(this.getGameState(), requestId) passes the function itself instead of the current GameSnapshot. All AI agents expect a snapshot and call toSerializableSnapshot(gameState)/read fields like gameState.board, so this call yields an empty object (or undefined fields) and the worker will error; TurnManager then falls back to the auto action. In practice, any AI turn will never compute a real move and will always degrade to the timeout/auto path. Passing state or this.getGameState() fixes the runtime behavior.
Useful? React with 👍 / 👎.
| const dist = Math.abs(to.x - selected.x) + Math.abs(to.y - selected.y) | ||
| const newSteps = stepsTaken + dist | ||
| const newSteps = stepsTaken + dist // Approximation. `pathResult.path.length - 1` is better. | ||
| // Use path length if available | ||
| const actualSteps = pathResult ? pathResult.path.length : newSteps | ||
|
|
There was a problem hiding this comment.
actualSteps is set to pathResult.path.length, but getPath includes the starting position in path, so a one-step move yields actualSteps === 2. That immediately fails the actualSteps < 2 check and sets stepsTaken to 2, which blocks the allowed second step in the same turn. This regression makes two-step moves impossible whenever getPath is used; use pathResult.path.length - 1 (or keep the previous dist logic) to preserve the 0/1/2-step rule.
Useful? React with 👍 / 👎.
| const pathResult = getPath(selected, to, state.board, 2) | ||
| const actualSteps = pathResult | ||
| ? pathResult.path.length | ||
| : stepsTaken + Math.abs(to.x - selected.x) + Math.abs(to.y - selected.y) |
There was a problem hiding this comment.
Same issue as the single-player store: pathResult.path.length counts the starting cell, so a single-step move sets actualSteps to 2 and prevents any follow-up step (actualSteps < 2 is false). This makes the multiplayer move logic incorrectly restrict turns to at most one step. Use pathResult.path.length - 1 when converting the path to step count.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements by adding online multiplayer functionality and expanding local gameplay to support 3 and 4 players. Key changes include the integration of peerjs and related WebRTC dependencies for peer-to-peer communication, new UI components for multiplayer lobbies (MultiplayerMenu) and games (MultiplayerGame), and updates to the main App.tsx to manage game modes. The core game logic has been refactored to support multiple players, including updates to AI evaluation (minimax-ai.ts), game state management (gameStore.test.ts, gameState.ts, index.ts), and turn progression. AI agents (AIWorker.ts, MinimaxAgent.ts, RandomAgent.ts) now incorporate requestId and cancellation mechanisms to prevent stale actions in asynchronous environments, and the TurnManager was updated to handle these new agent behaviors and ignore late replies. A new 'wall break' mechanic for 3/4 player games has been introduced, allowing players to remove walls once per game, with corresponding UI and state management. Localization files (en.json, es.json, ja.json, ko.json, zh-Hans.json, zh-Hant.json) have been updated to include new multiplayer strings and player names. Additionally, new test files were added to cover the TurnManager's race condition handling, worker interactions, and MinimaxAI's multiplayer logic. Configuration files (.gitignore, tsconfig.app.json, tsconfig.json, vite.config.ts, vitest.config.ts) were adjusted for new tooling artifacts and path aliases. A review comment highlighted that the applyActionSequence function in src/store/index.ts duplicates core action application logic, suggesting a refactor to a central private function for better maintainability. Another comment pointed out a redundant assertion in src/store/gameStore.test.ts that should be removed.
| // Atomically apply an action (move/place/wall) and any follow-up actions | ||
| // in a single state mutation. This prevents intermediate UI updates where | ||
| // the board reflects a moved piece but the turn hasn't been advanced yet. | ||
| applyActionSequence(action: import('@/lib/types').PlayerAction | undefined) { |
There was a problem hiding this comment.
The applyActionSequence function duplicates the core logic for applying actions (place, move, wall) which is also present in the individual action handlers (placeStone, moveTo, buildWall). This code duplication can lead to maintenance issues, as a change in one place might need to be manually synchronized in the other.
Consider refactoring this to a central, private function that both applyActionSequence and the individual handlers can call to apply the state changes for an action. This would centralize the game rules logic and improve maintainability.
| result.current.undo() | ||
| }) | ||
| expect(result.current.board[0][0].stone).toBe(PLAYER_LIST[0]) | ||
| expect(result.current.board[0][0].stone).toBe(null) |
There was a problem hiding this comment.
This assertion is a duplicate of the one on the previous line. It should be removed.
1 participant
Summary:\n- Add data-testid attributes to Board and Cell for stable E2E selectors.\n- Add Playwright probe script at .sisyphus/evidence/playwright-repro/run-e2e-probe.cjs to locate board and capture artifacts.\n\nHow to run probe locally:\n1. npm run dev\n2. node .sisyphus/evidence/playwright-repro/run-e2e-probe.cjs\n\nArtifacts saved to .sisyphus/evidence/playwright-repro/ after run.\n