-
Notifications
You must be signed in to change notification settings - Fork 4
201 server host reassignment #254
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?
Conversation
Added a priority queue to the api Added a sidebar to the setup page to show when people are in line. Allows for people to set their own names and kicks them out when they disconnect. Need to: Work on host switching Add queue to the in-progress game page.
and ran prettier again
Did you know: our web socket has a share function that allows multiple connections! without it, any connections added that are repeats will be ignored. Anyways the queue screen works and is now in the queue file.
Made it so someone is removed from the queue if they are disconnected for 2 seconds aka they close the tab, not just reload.
Allows the host to be reassigned based on who is first in line. Will end a game if someone disconnects for over 5 seconds. Need to: Move people to the end of the line after a game. Allow people to choose their names. Make the queue phone friendly.
Allows queue to move hosts when the game ends. Tested with 5 clients. Need to: Allow users to choose a name. make queue ui look better
Allow users to choose their name that shows up in queue To do: make ui look good
Added better formatting for the sidebar in both desktop and phone. Made the name box fill with previously inputted name
adjusted the formatting for the sidebar in games so it doesn't get hidden under the navbar
made the code for managing new hosts clearer. removed a duplicate loading spinner in the sidebar
changed media size and added padding to fix queue overlap
removed an unused import
Added all the dark mode functions to the sidebar and waiting screens. Moved the theme buttons to a variable for use in multiple places.
apparently JSX.Elements are required to be capitalized
@ymmot239 you cooked with this would be sick if you could make it so the join queue stuff doesn't show up when you are the host, and also shows a clearer indication of whether you are the host / where you are in the queue ![]() going to add more comments with some nits but great work so far (I also don't know if this is super high priority rn) |
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.
some tiny things
console.log("Server is listening on port 3000."); | ||
}); | ||
|
||
app.addListener("beforeunload", () => {}); |
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.
does this need to be here
@@ -0,0 +1,57 @@ | |||
// adapted from https://itnext.io/priority-queue-in-typescript-6ef23116901 | |||
/** | |||
* An priority queue class since ts doesn't have one |
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.
can we rename this to be queue.ts instead of queue.tsx
* returns a success message | ||
*/ | ||
apiRouter.post("/start-human-game", async (req, res) => { | ||
onlyOnce = true; |
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.
what is onlyOnce
used for
onMessage(message); | ||
} | ||
}, | ||
share: true, |
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.
what this do
} | ||
|
||
return ( | ||
<div |
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.
is there any way we can do this with mostly blueprint js components? just for visual consistency
} | ||
|
||
const queue = new PriorityQueue<string>(); | ||
const names = new Map<string, string>(); |
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.
Am I correct in assuming this is a map of user cookie IDs to the names that they enter when joining the queue? It would be great if we could make that clearer with a comment or renaming this variable.
export function ThemeButtons(props): JSX.Element { | ||
props; |
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.
Remove props, since we don't use them.
"get-queue", | ||
async () => { | ||
return get("/get-queue").then((newQueue) => { | ||
setQueue(newQueue); |
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.
I might not be reading this right, but I don't get why we store the queue information twice, once in the useState
hook and once in data
associated with this useEffectQuery
hook.
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 implements a player queue system for chess games with automatic host reassignment when players disconnect. The system allows spectators to join a queue and be automatically promoted to players when spots become available.
- Adds a priority queue data structure for managing player waitlists
- Implements automatic host/client reassignment logic when games end or players disconnect
- Creates a sidebar UI component for displaying the queue and allowing users to join
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
src/server/main.ts | Adds empty event listener for beforeunload |
src/server/api/queue.tsx | Implements PriorityQueue class for managing player queue |
src/server/api/managers.ts | Updates setGameManager to accept null values |
src/server/api/client-manager.ts | Adds methods for removing players and checking player status |
src/server/api/api.ts | Core queue management logic and WebSocket handling |
src/common/message/parse-message.ts | Adds parsing for new queue message types |
src/common/message/message.ts | Defines new message types for queue operations |
src/common/message/game-message.ts | Implements JoinQueue and UpdateQueue message classes |
src/client/setup/sidebar.tsx | Creates sidebar component for queue display and interaction |
src/client/setup/setup.tsx | Extracts theme buttons into reusable component |
src/client/setup/setup-base.tsx | Integrates sidebar into setup layout |
src/client/setup/lobby.tsx | Updates lobby display with queue-aware messaging |
src/client/index.scss | Adds responsive CSS for sidebar layout |
src/client/game/game.tsx | Integrates sidebar into game view |
src/client/api.ts | Enables WebSocket sharing for multiple components |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
console.log("Server is listening on port 3000."); | ||
}); | ||
|
||
app.addListener("beforeunload", () => {}); |
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 'beforeunload' event is a browser window event, not an Express app event. Express apps don't have this event listener. This line should be removed or the intended functionality should be implemented differently.
app.addListener("beforeunload", () => {}); |
Copilot uses AI. Check for mistakes.
public popInd(num: number | undefined): T | undefined { | ||
if (num !== undefined && this.data !== undefined) { | ||
const data = this.data[num][1]; | ||
this.data.slice(0, num).concat(this.data.slice(num + 1, -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.
This line creates a new array but doesn't assign it back to this.data, so the removal operation has no effect. It should be: this.data = this.data.slice(0, num).concat(this.data.slice(num + 1));
this.data.slice(0, num).concat(this.data.slice(num + 1, -1)); | |
this.data = this.data.slice(0, num).concat(this.data.slice(num + 1)); |
Copilot uses AI. Check for mistakes.
/** | ||
* gets the name associated with the request cookie | ||
*/ | ||
apiRouter.get("/get-name", (req, res) => { | ||
if (names) return res.send({ message: names.get(req.cookies.id) }); | ||
else return res.send(""); | ||
}); | ||
|
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 route handler is duplicated (lines 280-283 and 288-291). Remove one of the duplicate implementations.
/** | |
* gets the name associated with the request cookie | |
*/ | |
apiRouter.get("/get-name", (req, res) => { | |
if (names) return res.send({ message: names.get(req.cookies.id) }); | |
else return res.send(""); | |
}); |
Copilot uses AI. Check for mistakes.
console.log(names.isError); | ||
} | ||
//ts wanted me to do something with my data | ||
data; |
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 is a no-op statement that serves no purpose. If the intent is to satisfy TypeScript's unused variable warning, use void data; or remove the variable assignment altogether.
data; | |
void data; |
Copilot uses AI. Check for mistakes.
export function ThemeButtons(props): JSX.Element { | ||
props; |
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 props parameter should be typed (e.g., props: Record<string, never> for empty props) and the no-op statement props; should be removed or replaced with void props; if needed to suppress warnings.
export function ThemeButtons(props): JSX.Element { | |
props; | |
export function ThemeButtons(props: Record<string, never>): JSX.Element { | |
// no props expected |
Copilot uses AI. Check for mistakes.
data.clientType === ClientType.CLIENT ? | ||
"Waiting For Host to Start" | ||
: "Waiting in line" |
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.
[nitpick] The ternary operator formatting is inconsistent. Either use standard format (condition ? value1 : value2) or align the colons properly for better readability.
data.clientType === ClientType.CLIENT ? | |
"Waiting For Host to Start" | |
: "Waiting in line" | |
data.clientType === ClientType.CLIENT | |
? "Waiting For Host to Start" | |
: "Waiting in line" |
Copilot uses AI. Check for mistakes.
No description provided.