-
Notifications
You must be signed in to change notification settings - Fork 4
Admin pause play #261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Admin pause play #261
Conversation
added pause dialogs and messages to handle the client-side pause notification
Added a notification dialog to give the user information eg. game has been unpaused, puzzle info, line changes, etc.
added various gates to robot movement based on if the game is paused. Allowed pausing/unpausing through an api call
added a stop for promises in progress also added some hotfixes for pause
saved game state as well as robot positions in save files
allowed pause to rollback the robot positions. Also allowed saveGame to load from a saved positions, so hopefully less errors
Forced clients to be paused even on join or reload.
4682207
to
e166cf9
Compare
Added back some functions that were removed during a merge from main. Also removed some unused functions in executor.
Fixed the bug with executors not running all the commands Also cleaned up the paused api being sent to the clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces the ability to pause and unpause running chess games through admin controls. When paused, all robots are stopped, clients receive pause dialogs, and move execution is prevented. When unpaused, the game state rolls back to the previous position to reset robot states.
Key changes:
- Added pause/unpause API endpoints with robot control integration
- Enhanced save system to track current and previous game positions with robot locations
- Updated command execution to respect pause state and prevent moves during pause
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
File | Description |
---|---|
src/server/robot/robot.ts | Added sendStopPacket method to halt robot movement |
src/server/robot/robot-manager.ts | Added stopAllRobots method to stop all active robots |
src/server/command/executor.ts | Modified command execution to check pause state and added backlog management |
src/server/command/command.ts | Updated parallel and sequential command groups to respect pause state |
src/server/api/save-manager.ts | Enhanced save format to include FEN positions and robot locations |
src/server/api/managers.ts | Added global gamePaused flag |
src/server/api/game-manager.ts | Updated game managers to check pause state before processing moves |
src/server/api/api.ts | Added pause/unpause endpoints and robot position restoration logic |
src/common/game-end-reasons.ts | Added GAME_PAUSED and GAME_UNPAUSED hold reasons |
src/client/game/game.tsx | Added pause state handling and pause dialogs |
src/client/game/admin-dialogs.tsx | Created pause and notification dialog components |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/server/api/save-manager.ts
Outdated
*/ | ||
|
||
import { Side } from "../../common/game-types"; | ||
2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line contains an invalid standalone number '2' that will cause a syntax error. It should be removed.
2; |
Copilot uses AI. Check for mistakes.
src/server/command/executor.ts
Outdated
return this.runningCommands.forEach((command) => { | ||
command.execute().finally(() => { | ||
this.oldCommands.unshift(command); | ||
const index = this.runningCommands.indexOf(command); | ||
if (index >= 0) { | ||
this.runningCommands.splice(index, 1); | ||
} | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forEach returns undefined, but this function declares a Promise return type. The forEach callback executes asynchronously but the function returns immediately. Use Promise.all with map instead of forEach to properly handle async execution.
return this.runningCommands.forEach((command) => { | |
command.execute().finally(() => { | |
this.oldCommands.unshift(command); | |
const index = this.runningCommands.indexOf(command); | |
if (index >= 0) { | |
this.runningCommands.splice(index, 1); | |
} | |
}); | |
}); | |
await Promise.all( | |
this.runningCommands.map((command) => | |
command.execute().finally(() => { | |
this.oldCommands.unshift(command); | |
const index = this.runningCommands.indexOf(command); | |
if (index >= 0) { | |
this.runningCommands.splice(index, 1); | |
} | |
}) | |
) | |
); |
Copilot uses AI. Check for mistakes.
src/server/command/command.ts
Outdated
const promises = this.commands.map((move) => move.execute()); | ||
const promises = this.commands.map((move) => { | ||
if (!gamePaused.flag) return move.execute(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When gamePaused.flag is true, the map callback returns undefined, creating an array with undefined values that are passed to Promise.all. This will cause Promise.all to resolve immediately with undefined values instead of waiting for paused commands. Consider filtering out undefined values or using a different approach.
}); | |
}).filter(Boolean); |
Copilot uses AI. Check for mistakes.
for (const command of this.commands) { | ||
promise = promise.then(() => command.execute()); | ||
promise = promise.then(() => { | ||
if (!gamePaused.flag) return command.execute(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When gamePaused.flag is true, the promise chain returns undefined instead of a resolved promise. This breaks the sequential execution chain. The function should return Promise.resolve() when paused to maintain the chain.
if (!gamePaused.flag) return command.execute(); | |
if (!gamePaused.flag) return command.execute(); | |
return Promise.resolve(); |
Copilot uses AI. Check for mistakes.
src/client/game/game.tsx
Outdated
sendMessage(new MoveMessage(move)); | ||
} | ||
: (move: Move): void => { | ||
move; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The statement 'move;' has no effect and creates dead code. Consider using '() => {}' for the no-op function or add a comment explaining why the parameter is unused.
move; | |
// Parameter 'move' is unused because moves are ignored when the game is paused. |
Copilot uses AI. Check for mistakes.
* Pause the game | ||
* Todo: add authentication instead of an exposed pause call | ||
*/ | ||
apiRouter.get("/pause-game", (_, res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pause endpoint is publicly accessible without authentication as noted in the TODO. This allows anyone to pause games, which could be abused to disrupt gameplay.
Copilot uses AI. Check for mistakes.
*/ | ||
apiRouter.get("/unpause-game", async (_, res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unpause endpoint is publicly accessible without authentication as noted in the TODO. This allows anyone to unpause games, which could be abused to disrupt gameplay.
*/ | |
apiRouter.get("/unpause-game", async (_, res) => { | |
* Now requires a valid x-api-key header. | |
*/ | |
apiRouter.get("/unpause-game", async (req, res) => { | |
// Simple authentication check using x-api-key header | |
const API_KEY = "CHANGE_ME_TO_A_SECRET_KEY"; // Replace with env variable if available | |
const clientKey = req.headers["x-api-key"]; | |
if (clientKey !== API_KEY) { | |
return res.status(401).send({ message: "Unauthorized" }); | |
} |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Added various changes recommended in the PR, such as promise resolution and execution problems.
Added the ability to pause running games.
Will send a message to all clients displaying the paused dialogs, and will attempt to stop all robots.
When unpaused, will rollback to the last move to reset the bots.
Also allows saves to load robots from positions.