Conversation
Contributor
WalkthroughAdds optional friend lists to player schemas and runtime types, threads friend data from authentication into Client and GameRunner, uses friend-aware placement in team assignment (nation-first, friend-count tie-break), and broadcasts resolved friend links in the game-start payload. Tests and fixtures updated accordingly. ChangesFriend System Implementation
Sequence DiagramsequenceDiagram
participant WebSocket Handler
participant getUserMe API
participant Worker
participant Client
participant GameRunner
participant PlayerInfo
WebSocket Handler->>getUserMe API: verify token
getUserMe API-->>Worker: player{publicId, friends}
Worker->>Client: new Client(publicId, friends)
Client->>GameRunner: addPlayer
GameRunner->>PlayerInfo: new PlayerInfo(..., friends)
Note over PlayerInfo: friends field used for team assignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/server/KickPlayerAuthorization.test.ts (1)
37-56: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace direct
new Client(...)wiring withsetup()-based test construction.Line 54 and Line 55 were added only to keep up with constructor changes; this is exactly the drift
setup()avoids. Migrating this helper totests/util/Setup.tswill make these authorization tests more stable.As per coding guidelines
tests/**/*.test.ts: "Use thesetup()helper fromtests/util/Setup.tsto create test game instances and exercise core simulation directly."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server/KickPlayerAuthorization.test.ts` around lines 37 - 56, The makeClient helper currently instantiates Client directly (the new Client(...) call in makeClient) and manually passes constructor args to work around signature drift; replace this by using the setup() test helper from tests/util/Setup.ts to construct the test game/client state instead of new Client. Update makeClient to call setup() (or the specific helper it exposes) to create a game/session and derive a Client instance or handle from that setup output, remove the manual constructor wiring (including the extra persistent params added on lines matching the current diff), and adapt tests to use the setup-provided client/ws objects so future constructor changes are handled by the shared setup helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/ApiSchemas.ts`:
- Line 120: The UserMeResponseSchema currently requires friends (friends:
z.array(z.string())), which breaks clients that don't send it; update the schema
for UserMeResponseSchema so friends is optional and has a safe default (e.g.,
make the friends field optional and default to an empty array) to preserve
backward compatibility and avoid rejecting valid /me payloads. Locate
UserMeResponseSchema and change the friends declaration to be optional with a
default value so downstream code still receives an array when the field is
absent.
In `@src/core/game/TeamAssignment.ts`:
- Around line 63-67: The loop in TeamAssignment that unions players on one-way
edges should only union when the friendship is mutual: inside the nested loop
over players and friendID (variables players, p, friendID, byClientID, friend),
check that friend !== undefined and that friend.friends includes p.id (or use a
Set lookup) before calling uf.union(p.id, friend.id); update the condition that
currently does uf.union(p.id, friend.id) to require this mutual check so only
reciprocated friendships create unions.
In `@tests/TeamAssignment.test.ts`:
- Around line 20-34: The helper createPlayerWithFriends currently constructs
PlayerInfo directly (new PlayerInfo(...)), which is brittle; instead invoke the
shared setup() test helper to create players so tests follow core simulation
wiring. Replace the body of createPlayerWithFriends to call setup() and the
appropriate player-creation helper (the setup instance's player creation/add
method) to produce a PlayerInfo configured with the given id, friends and
optional clan, and return that object rather than constructing PlayerInfo
yourself.
- Around line 28-29: Update the test data in TeamAssignment.test.ts to include
one case where clientID and PlayerInfo.id are different (e.g., clientID =
"client-1" and PlayerInfo.id = "player-1") instead of using the same value for
both; place this in the same test array/fixture used by the existing tests and
adjust the assertions to explicitly verify that grouping/lookup uses clientID
(not PlayerInfo.id) for friend grouping (reference the clientID and
PlayerInfo.id fields in the test data and the grouping/assertion logic to locate
the change).
---
Outside diff comments:
In `@tests/server/KickPlayerAuthorization.test.ts`:
- Around line 37-56: The makeClient helper currently instantiates Client
directly (the new Client(...) call in makeClient) and manually passes
constructor args to work around signature drift; replace this by using the
setup() test helper from tests/util/Setup.ts to construct the test game/client
state instead of new Client. Update makeClient to call setup() (or the specific
helper it exposes) to create a game/session and derive a Client instance or
handle from that setup output, remove the manual constructor wiring (including
the extra persistent params added on lines matching the current diff), and adapt
tests to use the setup-provided client/ws objects so future constructor changes
are handled by the shared setup helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63a2a947-0604-4ccd-9fd7-bf095fe7236f
📒 Files selected for processing (11)
src/core/ApiSchemas.tssrc/core/GameRunner.tssrc/core/Schemas.tssrc/core/game/Game.tssrc/core/game/TeamAssignment.tssrc/server/Client.tssrc/server/GameServer.tssrc/server/Worker.tstests/ResolveCosmetics.test.tstests/TeamAssignment.test.tstests/server/KickPlayerAuthorization.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/core/ApiSchemas.ts (1)
120-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
friendsbackward-compatible in/meschema.Line 120 currently requires
friends. If/users/@meomits this field,safeParsefails and join auth is rejected. Please default it to an empty array.Suggested fix
- friends: z.array(z.string()), + friends: z.array(z.string()).default([]),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/ApiSchemas.ts` at line 120, The /me response schema requires the friends field as z.array(z.string()), which breaks safeParse when the endpoint omits it; modify the schema entry for friends in the /me schema (the object that currently contains "friends: z.array(z.string())") to be optional with a default empty array (e.g., use z.array(z.string()).optional().default([]) or equivalent) so missing friends values parse as an empty list and join auth no longer fails.tests/TeamAssignment.test.ts (2)
20-34: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
setup()-based player creation in this helper.
createPlayerWithFriendsstill constructsPlayerInfodirectly. Please create players throughtests/util/Setup.tsso this suite stays aligned with simulation wiring.As per coding guidelines
tests/**/*.test.ts: "Use thesetup()helper fromtests/util/Setup.tsto create test game instances and exercise core simulation directly."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/TeamAssignment.test.ts` around lines 20 - 34, The helper createPlayerWithFriends directly constructs PlayerInfo instances instead of using the test harness; update createPlayerWithFriends to create players via the setup() helper from tests/util/Setup.ts (e.g., call setup() or its player-creation API) so the returned Player records are created through the simulation wiring rather than new PlayerInfo(...); replace the new PlayerInfo(...) usage and ensure clan, friends, id/clientID and PlayerType are passed through to the setup-based creation so tests continue to receive equivalent Player objects.
28-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd one scenario where
clientIDdiffers fromPlayerInfo.id.All new friend tests keep
clientID === id, so they won’t catch accidental grouping by the wrong identifier.As per coding guidelines
tests/**/*.test.ts: "Use thesetup()helper fromtests/util/Setup.tsto create test game instances and exercise core simulation directly."Also applies to: 186-290
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/TeamAssignment.test.ts` around lines 28 - 29, Add a test scenario in TeamAssignment.test.ts where the two identifiers differ by creating a player with distinct clientID and PlayerInfo.id (e.g., pass idA as PlayerInfo.id and idB as clientID) so the grouping logic is exercised against clientID rather than PlayerInfo.id; use the existing setup() test helper to instantiate the game/simulation and create players, update the section(s) around the existing entries that currently read "id, // clientID" and "id, // PlayerInfo.id" to supply different values, and assert that team assignment/grouping uses the clientID value (not PlayerInfo.id) to catch accidental grouping by the wrong identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/game/TeamAssignment.ts`:
- Around line 129-130: The loop currently iterates over
otherPlayers.concat(nationPlayers) which places non-nation players before
nations, breaking the intended deterministic nation-first ordering; change the
iteration to place nationPlayers before others (e.g., iterate over
nationPlayers.concat(otherPlayers) or run two loops: first over nationPlayers,
then otherPlayers) so that placePlayer is invoked for nations first.
---
Duplicate comments:
In `@src/core/ApiSchemas.ts`:
- Line 120: The /me response schema requires the friends field as
z.array(z.string()), which breaks safeParse when the endpoint omits it; modify
the schema entry for friends in the /me schema (the object that currently
contains "friends: z.array(z.string())") to be optional with a default empty
array (e.g., use z.array(z.string()).optional().default([]) or equivalent) so
missing friends values parse as an empty list and join auth no longer fails.
In `@tests/TeamAssignment.test.ts`:
- Around line 20-34: The helper createPlayerWithFriends directly constructs
PlayerInfo instances instead of using the test harness; update
createPlayerWithFriends to create players via the setup() helper from
tests/util/Setup.ts (e.g., call setup() or its player-creation API) so the
returned Player records are created through the simulation wiring rather than
new PlayerInfo(...); replace the new PlayerInfo(...) usage and ensure clan,
friends, id/clientID and PlayerType are passed through to the setup-based
creation so tests continue to receive equivalent Player objects.
- Around line 28-29: Add a test scenario in TeamAssignment.test.ts where the two
identifiers differ by creating a player with distinct clientID and PlayerInfo.id
(e.g., pass idA as PlayerInfo.id and idB as clientID) so the grouping logic is
exercised against clientID rather than PlayerInfo.id; use the existing setup()
test helper to instantiate the game/simulation and create players, update the
section(s) around the existing entries that currently read "id, // clientID" and
"id, // PlayerInfo.id" to supply different values, and assert that team
assignment/grouping uses the clientID value (not PlayerInfo.id) to catch
accidental grouping by the wrong identifier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4cde62e2-4ee2-4fd6-8a75-ae391ee8e2c7
📒 Files selected for processing (11)
src/core/ApiSchemas.tssrc/core/GameRunner.tssrc/core/Schemas.tssrc/core/game/Game.tssrc/core/game/TeamAssignment.tssrc/server/Client.tssrc/server/GameServer.tssrc/server/Worker.tstests/ResolveCosmetics.test.tstests/TeamAssignment.test.tstests/server/KickPlayerAuthorization.test.ts
Comment on lines
+129
to
+130
| for (const p of otherPlayers.concat(nationPlayers)) { | ||
| placePlayer(p); |
Contributor
There was a problem hiding this comment.
Place nations first to keep intended deterministic ordering.
Lines 129-130 currently place non-nation players before nations. That reverses the intended nation-first placement and can change team outcomes.
Suggested fix
- for (const p of otherPlayers.concat(nationPlayers)) {
+ for (const p of nationPlayers.concat(otherPlayers)) {
placePlayer(p);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const p of otherPlayers.concat(nationPlayers)) { | |
| placePlayer(p); | |
| for (const p of nationPlayers.concat(otherPlayers)) { | |
| placePlayer(p); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/game/TeamAssignment.ts` around lines 129 - 130, The loop currently
iterates over otherPlayers.concat(nationPlayers) which places non-nation players
before nations, breaking the intended deterministic nation-first ordering;
change the iteration to place nationPlayers before others (e.g., iterate over
nationPlayers.concat(otherPlayers) or run two loops: first over nationPlayers,
then otherPlayers) so that placePlayer is invoked for nations first.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/game/TeamAssignment.ts (1)
129-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPlace nations before other players for deterministic ordering.
The PR description mentions "deterministic nation-first ordering," but
otherPlayers.concat(nationPlayers)places non-nation players first. Swap the order.- for (const p of otherPlayers.concat(nationPlayers)) { + for (const p of nationPlayers.concat(otherPlayers)) { placePlayer(p); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/game/TeamAssignment.ts` around lines 129 - 131, The loop currently iterates otherPlayers.concat(nationPlayers) which places non-nation players first; swap the concatenation order so nationPlayers come before otherPlayers to enforce deterministic nation-first ordering—update the iteration in TeamAssignment (the for loop that calls placePlayer(p)) to iterate nationPlayers.concat(otherPlayers) instead so placePlayer is invoked for nation players first.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/core/game/TeamAssignment.ts`:
- Around line 129-131: The loop currently iterates
otherPlayers.concat(nationPlayers) which places non-nation players first; swap
the concatenation order so nationPlayers come before otherPlayers to enforce
deterministic nation-first ordering—update the iteration in TeamAssignment (the
for loop that calls placePlayer(p)) to iterate
nationPlayers.concat(otherPlayers) instead so placePlayer is invoked for nation
players first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8faede44-f66d-4379-9831-96ccf8f92f10
📒 Files selected for processing (11)
src/core/ApiSchemas.tssrc/core/GameRunner.tssrc/core/Schemas.tssrc/core/game/Game.tssrc/core/game/TeamAssignment.tssrc/server/Client.tssrc/server/GameServer.tssrc/server/Worker.tstests/ResolveCosmetics.test.tstests/TeamAssignment.test.tstests/server/KickPlayerAuthorization.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/ResolveCosmetics.test.ts
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
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
1 participant
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.
Fixes #3911
Description:
publicIdandfriendsfromgetUserMe()and includes each player's in-game friendclientIDs inPlayerSchemaon game startgetUserMeis stalePlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan