Conversation
big merge, please double check things
Closed
cjs8487
requested changes
Aug 31, 2026
Owner
There was a problem hiding this comment.
Thinking a little bit from the frontend perspective, I have a handful of stray thoughts
- I'm back to thinking that maybe we should treat spectators as an implicit team. It would allow consumers to not have to think about spectators as a separate entity (since they'd all be grouped under the spectator team) but with the existence of a toggle for teams, I think this becomes awkward
- I call it out in a few spots, but it may be worth considering if we should include which player on a team performed an action alongside the action, both in the database and in generated chat messages. Doing it in the database if we want this will likely save headache in protocol v2.
Other miscellaneous comments
- Please use
array.forEachinstead offor (const elem of array). I know a lot of development environments have regressed back to suggesting that style of loop, but it's not the way we've been writing loops in this codebase. technically, it can require a polyfill, but we're not running the APi in the browser so it's not a major concern. ESLint would typically flag this if it was working correctly, at least in browser projects - There's quite a few places where the usage of helper methods is inconsistent. Go through with a fine toothed comb and make sure that helpers added in later iterations were properly used everywhere so there's no gotchas in 6 months when we inevitably need to make a change to it.
- It's possible I missed it in the sea of changes, but I don't see any handling for creating a team if aplayer switches from spectating to playing, or handling them leaving a team if they switch from playing to sepctatin. Both of these are existing options and separate actions, so we should take care to not break them
cjs8487
reviewed
Aug 31, 2026
cjs8487
left a comment
cjs8487
left a comment
Owner
There was a problem hiding this comment.
Sent my first review too early :/
| } from '../../database/Rooms'; | ||
|
|
||
| jest.mock('../../database/Rooms', () => ({ | ||
| addChangeColorAction: jest.fn().mockResolvedValue(undefined), |
Owner
There was a problem hiding this comment.
I've been putting mocks for the database directly into the respective mocks folder, and calling the module level mock for it from the setup script. This lets all the tests use the same mocks, which generally is the better paradigm, unless a test suite really needs specific mocks that don't match the other test. If you don't do it here, I may very well end up moving it, but I'd rather if we started some consistency
Collaborator
Author
There was a problem hiding this comment.
Which mocks folder? I've neither seen one in main nor in your test branch. Needles to say it's not in my branch either
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.