Skip to content

Support Teams - #199

Open
Floha258 wants to merge 13 commits into
mainfrom
teams
Open

Support Teams#199
Floha258 wants to merge 13 commits into
mainfrom
teams

Conversation

@Floha258

Copy link
Copy Markdown
Collaborator

While the title is as simple as saying "Support Teams" the underlying code changes are pretty big, refactoring the entire player and room model.

@Floha258 Floha258 self-assigned this Jul 12, 2026

@cjs8487 cjs8487 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the changes included are solid enough so far. Need to finish cleaning up the database and some of the room functionality, and then iron out the final details over websocket and this should be good to go

Comment thread api/prisma/migrations/20260515200019_refactor_for_teams/migration.sql Outdated
Comment thread api/src/auth/RoomAuth.ts Outdated
Comment thread api/src/core/Player.ts Outdated
Comment thread api/src/core/Room.ts Outdated
Comment thread api/src/core/Room.ts Outdated
let newPlayer = false;
if (this.players.has(auth.playerId)) {
player = this.players.get(auth.playerId);
let playerIsAuthed = false;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can largely be the same as in current, just using getAllPlayers aince you can work backwards from the playerId to the team

Comment thread api/src/core/Room.ts Outdated
Comment thread api/src/database/Rooms.ts
Comment thread api/src/core/Room.ts Outdated
Comment thread api/src/core/Player.ts Outdated
@Floha258
Floha258 marked this pull request as ready for review July 15, 2026 21:11

@cjs8487 cjs8487 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of additional stray thoughts that apply in too many places to reasonably call out in individual comments.

  • We should probably have a helper function in Room to get a player by id rather than having to write out the whole getAllPlayers...find... each time.
  • Should we have a teams toggle for the room and corresponding helpers that determine how we display/format messages? So that, for example, in single player, we're not constantly saying "Player X's Team" rather than "Player X"

// is not available in the current Prisma Client generation.
// We also need to fetch the roomId to create the team in the correct room.
const players: any[] = await tx.$queryRawUnsafe(
'SELECT id, spectator, "roomId", nickname FROM "Player"'

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're no longer dropping the spectator column in the current migration, you should be able to write this as a properly typed sequence fully within prisma

Comment thread api/src/core/Room.ts Outdated
Comment thread api/src/core/Room.ts Outdated
Comment thread api/src/core/Room.ts Outdated
handleSocketClose(ws: WebSocket) {
let player: Player | undefined;
for (const p of this.players.values()) {
for (const p of this.getAllPlayers()) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge stickler on this, but I believe it's generally better to use .forEach rather than a for ... of loop

Comment thread api/src/core/Room.ts Outdated
contents: player.nickname,
color: player.color,
contents: team.name,
// TODO: Which color should this be?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unresolved TODO, if we're doing a full conversion of player -> team generally I would think this should just be team.color

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally wanted to leave colors on player so that different players can have different colors

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, how are we going to show marked goals in the frontend? I don't have a problem with maintaining color per player, but I do think we definitely also need a team level color. I haven't fully mapped out what the frontend will be yet, but I think if anything the ability to view marks per-player will be a toggle of some sort

Comment thread api/src/core/Team.ts
import { HiddenCell, RevealedCell, Team as TeamData } from '@playbingo/types';
import { computeRevealedMask, rowColToMask } from '../util/RoomUtils';

export default class Team {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Teams should probably have a color

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, my idea was to leave colors on players so that each player in a team can have a different colour if wanted

Comment thread api/src/core/Team.ts
Comment thread api/src/core/Player.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants