feat(adminbot): let create_game pin teams - #4944
Open
Zixer1 wants to merge 1 commit into
Open
Conversation
GameManager.createGame already accepts matchmakingTeams and GameServer already resolves a client's pinned slot from it, but the admin-bot route called createGame with three arguments, so it was always undefined. A bot could create a Team game but not say who plays with whom — the balancer decided, which is no use for a tournament running fixed pairs. create_game now takes an optional teams: string[][] of publicIds. It is read alongside the config rather than through GameConfig, for the same reason as listed: the schema parse strips it, so it can never be set later via update_game_config. Rejects teams in FFA (that mode never runs assignTeams, so the pin would be silently inert) and a publicId appearing in two teams (findIndex takes the first, so the caller would get a team it did not ask for).
Contributor
WalkthroughThe admin bot game creation route now accepts optional team assignments. It enforces team and member limits, requires Team mode, rejects duplicate public IDs, forwards valid assignments to ChangesAdmin bot team pinning
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AdminBot
participant AdminBotRoutes
participant GameManager
AdminBot->>AdminBotRoutes: Submit game creation request
AdminBotRoutes->>AdminBotRoutes: Validate team assignments and game mode
AdminBotRoutes->>GameManager: Call createGame with validated teams
GameManager-->>AdminBotRoutes: Return created game
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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: 2
🤖 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/server/AdminBotRoutes.ts`:
- Around line 119-139: Update the validation and team-assignment error responses
in the route handler to pass all user-visible messages through translateText()
before returning them: the z.prettifyError(teamsParsed.error) result, the
gameMode Team message, and the duplicate publicId message. Preserve the existing
status codes and error content while using the established translation context
and interpolation pattern.
In `@tests/server/AdminBotCreateTeams.test.ts`:
- Around line 8-37: Replace the mock-based captureCreateHandler tests with
setup() using a map from tests/testdata/maps/. Create the game through the real
GameManager and route flow, then exercise the resulting game to verify
matchmakingTeams reaches the game and produces the expected team assignment.
Remove the mocked Express and GameManager objects while preserving the route
behavior under test.
🪄 Autofix
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 Plus
Run ID: 93a05d6d-88ff-4515-904a-1dc8319cc96a
📒 Files selected for processing (2)
src/server/AdminBotRoutes.tstests/server/AdminBotCreateTeams.test.ts
Comment on lines
+119
to
+139
| if (!teamsParsed.success) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: z.prettifyError(teamsParsed.error) }); | ||
| } | ||
| const teams = teamsParsed.data.teams; | ||
| if (teams !== undefined) { | ||
| // FFA never runs assignTeams, so a pin there would be silently inert. | ||
| // Refuse rather than accept a request that cannot do what it asks. | ||
| if (config.gameMode !== GameMode.Team) { | ||
| return res.status(400).json({ error: "teams require gameMode Team" }); | ||
| } | ||
| // A publicId in two teams has no single answer (findIndex takes the first), | ||
| // so the caller would get a team it did not ask for. | ||
| const seen = new Set<string>(); | ||
| for (const team of teams) { | ||
| for (const publicId of team) { | ||
| if (seen.has(publicId)) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: `publicId in more than one team: ${publicId}` }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Localize the new API error text.
Lines 122, 129, and 139 return user-visible text without translateText(). Localize the Zod validation response and both literal error responses before returning them.
As per coding guidelines, “All user-visible text must go through translateText().”
🤖 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/server/AdminBotRoutes.ts` around lines 119 - 139, Update the validation
and team-assignment error responses in the route handler to pass all
user-visible messages through translateText() before returning them: the
z.prettifyError(teamsParsed.error) result, the gameMode Team message, and the
duplicate publicId message. Preserve the existing status codes and error content
while using the established translation context and interpolation pattern.
Source: Coding guidelines
Comment on lines
+8
to
+37
| function captureCreateHandler() { | ||
| const routes: Record<string, (req: any, res: any) => void> = {}; | ||
| const app: any = { | ||
| post(path: string, ...handlers: ((req: any, res: any) => void)[]) { | ||
| routes[path] = handlers[handlers.length - 1]; | ||
| }, | ||
| get() {}, | ||
| }; | ||
| const created: { | ||
| config?: Record<string, unknown>; | ||
| teams?: string[][]; | ||
| } = {}; | ||
| const gm: any = { | ||
| createGame( | ||
| _id: string, | ||
| config: Record<string, unknown>, | ||
| _creator?: string, | ||
| _startsAt?: number, | ||
| _publicGameType?: unknown, | ||
| matchmakingTeams?: string[][], | ||
| ) { | ||
| created.config = config; | ||
| created.teams = matchmakingTeams; | ||
| return { setListed: vi.fn(), gameInfo: () => ({ gameID: "x" }) }; | ||
| }, | ||
| }; | ||
| const log: any = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; | ||
| registerAdminBotRoutes({ app, gm, workerId: 0, log }); | ||
| return { handler: routes["/api/adminbot/create_game"], created }; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Test the route with a real game instance.
captureCreateHandler() replaces GameManager.createGame with a mock. The tests only verify captured arguments. They do not verify that matchmakingTeams reaches the game or affects team assignment.
Use setup() with a map from tests/testdata/maps/. Exercise the created game and team assignment without mocked GameManager or Express objects.
As per coding guidelines, “Use the setup() helper … and exercise core simulation directly rather than using mocks.”
🤖 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/AdminBotCreateTeams.test.ts` around lines 8 - 37, Replace the
mock-based captureCreateHandler tests with setup() using a map from
tests/testdata/maps/. Create the game through the real GameManager and route
flow, then exercise the resulting game to verify matchmakingTeams reaches the
game and produces the expected team assignment. Remove the mocked Express and
GameManager objects while preserving the route behavior under test.
Source: Coding guidelines
github-project-automation
Bot
moved this from Triage
to Development
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.
Lets a bot say who plays with whom in a Team game.
GameManager.createGamealready acceptsmatchmakingTeams, andGameServeralready resolves a client's pinned slot from it — but the admin-bot route calledcreateGamewith three arguments, so it was alwaysundefined. A bot could create a Team game but not control the pairings; the balancer decided, which is no use for a tournament running fixed pairs.create_gamenow takes an optionalteams: string[][]of publicIds, read alongside the config rather than throughGameConfig— same treatment aslisted, so it can't be set later viaupdate_game_config.Rejects two cases rather than accepting a request that can't do what it asks:
teamsin FFA, whereassignTeamsnever runs so the pin would be silently inertfindIndextakes the first and the caller would get a team they didn't ask forFollow-up (not here): teams can't be amended after create, so a player added to the allowlist pre-start can't be pinned to their partner.