Open
Conversation
Contributor
WalkthroughAdds admin-based moderation: client fetches user role and marks admins in the player UI; moderation components accept an Changes
Sequence DiagramsequenceDiagram
participant Client
participant GameRenderer
participant API
participant PlayerPanel
participant PlayerModerationModal
participant GameServer
Client->>GameRenderer: initialize renderer
GameRenderer->>API: userAuth() / getUserMe()
API-->>GameRenderer: user.me (claims.role = "admin")
GameRenderer->>PlayerPanel: playerPanel.setRole("admin")
PlayerPanel->>PlayerPanel: compute isAdmin (role === "admin" || "root")
PlayerPanel->>PlayerModerationModal: render with .isAdmin=${isAdmin}
Client->>PlayerPanel: click kick button
PlayerPanel->>GameServer: send kick_player intent (ws)
GameServer->>GameServer: authorize: isLobbyCreator OR isAdminRole(client.role)
alt authorized && caller is admin (not lobby creator)
GameServer->>GameServer: kickReason = KICK_REASON_ADMIN
else authorized && caller is lobby creator
GameServer->>GameServer: kickReason = KICK_REASON_LOBBY_CREATOR
else not authorized
GameServer-->>GameServer: log & ignore
end
GameServer->>GameServer: kickClient(target, kickReason)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Enables players with the admin flare to kick players in-game (not limited to the lobby creator), with corresponding UI support and updated translations/tests.
Changes:
- Server: allow
kick_playerintent when the authenticated client is either lobby creator or has theadminflare; emit different kick reasons. - Client: show moderation/kick UI for admins (and pass
isAdmininto the moderation modal). - Add/extend tests for kick authorization and admin moderation rendering; add
kick_reason.admintranslation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/server/KickPlayerAuthorization.test.ts | Adds server-side authorization tests for lobby creator/admin kick behavior. |
| tests/client/graphics/layers/PlayerPanelKick.test.ts | Updates and extends client tests for moderation visibility and modal canKick logic with admin support. |
| src/server/GameServer.ts | Implements admin authorization path + admin-specific kick reason. |
| src/client/graphics/layers/PlayerPanel.ts | Adds admin-based gating for moderation UI and wires isAdmin into the modal. |
| src/client/graphics/layers/PlayerModerationModal.ts | Allows kicking when isAdmin is true (still blocks self/non-human/etc.). |
| src/client/graphics/GameRenderer.ts | Fetches user-me flares to enable admin UI client-side. |
| resources/lang/en.json | Adds kick_reason.admin user-facing text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/graphics/GameRenderer.ts (1)
208-216: Add lightweight logging for flare fetch failures.Line 214 currently suppresses all errors, which makes admin-UI issues hard to debug. A small warn/debug log would improve supportability without changing behavior.
♻️ Suggested tweak
getUserMe() .then((userMe) => { if (userMe !== false) { playerPanel.flares = userMe.player.flares ?? []; } }) - .catch(() => { - // getUserMe failure — flares stay empty, admin UI stays hidden + .catch((error) => { + console.warn("Failed to load user flares for moderation UI", { + error: String(error), + }); + // flares stay empty, admin UI stays hidden });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/GameRenderer.ts` around lines 208 - 216, The catch block swallowing errors from getUserMe should log a lightweight warning so flare fetch failures are visible; update the .catch in GameRenderer where getUserMe() is called to accept an error parameter and call console.warn (or the existing client logger if available) with a short message and the error (e.g., "getUserMe failed while fetching flares:" , error) while keeping the current behavior of leaving playerPanel.flares empty and admin UI hidden.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/GameRenderer.ts`:
- Around line 208-216: The catch block swallowing errors from getUserMe should
log a lightweight warning so flare fetch failures are visible; update the .catch
in GameRenderer where getUserMe() is called to accept an error parameter and
call console.warn (or the existing client logger if available) with a short
message and the error (e.g., "getUserMe failed while fetching flares:" , error)
while keeping the current behavior of leaving playerPanel.flares empty and admin
UI hidden.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa00a91f-c212-44eb-ae05-1f26cc7f0cee
📒 Files selected for processing (7)
resources/lang/en.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/PlayerModerationModal.tssrc/client/graphics/layers/PlayerPanel.tssrc/server/GameServer.tstests/client/graphics/layers/PlayerPanelKick.test.tstests/server/KickPlayerAuthorization.test.ts
Collaborator
|
instead of using flares we are using roles. so the jwt will have field like: role:(root|admin|mod|flagged|banned) |
ryanbarlow97
force-pushed
the
improveingameadmin
branch
from
21f1346 to
5cbeecc
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/GameServer.ts (1)
394-434:⚠️ Potential issue | 🟠 MajorDon’t let admin kicks tear down the whole private lobby.
If Line 431 kicks the current
lobbyCreatorIDbefore the game starts, the existing close path at Lines 563-579 treats that as “host left” and closes the entire private lobby. That makes one admin moderation action remove every player, not just the target.Possible guard
const isLobbyCreator = client.clientID === this.lobbyCreatorID; const isAdmin = client.role === "admin" || client.role === "root"; + const isTargetLobbyCreator = + stampedIntent.target === this.lobbyCreatorID; // Check if the authenticated client is the lobby creator or admin if (!isLobbyCreator && !isAdmin) { this.log.warn( `Only lobby creator or admin can kick players`, @@ ); return; } + + if ( + isAdmin && + !isLobbyCreator && + isTargetLobbyCreator && + !this.hasStarted() && + !this.isPublic() + ) { + this.log.warn(`Cannot admin-kick lobby creator before start`, { + clientID: client.clientID, + target: stampedIntent.target, + gameID: this.id, + }); + return; + }This also needs a regression test for “admin kicks lobby creator in a private pre-start lobby.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/GameServer.ts` around lines 394 - 434, The current kick flow allows an admin to kick the lobby creator (lobbyCreatorID) in a private, pre-start game which later triggers the host-left teardown path; prevent that by adding a guard in the kick initiation: if stampedIntent.target === this.lobbyCreatorID && this.isPrivate && !this.started then disallow the kick (return with a warn) or require transferring lobby ownership first; update the logic around this.kickClient call and/or the on-client-left/host-left handler to ignore admin-initiated kicks of the host so it doesn't run the private-lobby teardown; also add a regression test "admin kicks lobby creator in a private pre-start lobby" to assert the lobby remains open and ownership/teardown behavior is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/server/GameServer.ts`:
- Around line 394-434: The current kick flow allows an admin to kick the lobby
creator (lobbyCreatorID) in a private, pre-start game which later triggers the
host-left teardown path; prevent that by adding a guard in the kick initiation:
if stampedIntent.target === this.lobbyCreatorID && this.isPrivate &&
!this.started then disallow the kick (return with a warn) or require
transferring lobby ownership first; update the logic around this.kickClient call
and/or the on-client-left/host-left handler to ignore admin-initiated kicks of
the host so it doesn't run the private-lobby teardown; also add a regression
test "admin kicks lobby creator in a private pre-start lobby" to assert the
lobby remains open and ownership/teardown behavior is correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 818fcc77-7e55-4ed0-8516-4cd42e3cf875
📒 Files selected for processing (10)
resources/lang/en.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/PlayerModerationModal.tssrc/client/graphics/layers/PlayerPanel.tssrc/core/ApiSchemas.tssrc/server/Client.tssrc/server/GameServer.tssrc/server/Worker.tstests/client/graphics/layers/PlayerPanelKick.test.tstests/server/KickPlayerAuthorization.test.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/graphics/GameRenderer.ts
- tests/client/graphics/layers/PlayerPanelKick.test.ts
ryanbarlow97
force-pushed
the
improveingameadmin
branch
from
5cbeecc to
71b3462
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/client/graphics/layers/PlayerPanel.ts (1)
81-83: Reuse the shared admin-role helper here.The server now relies on
isAdminRole()fromsrc/core/ApiSchemas.ts, but the panel hardcodes the same string list again. Importing the helper keeps the moderation button in sync with the real permission rule.♻️ Proposed refactor
+import { isAdminRole as hasAdminRole } from "../../../core/ApiSchemas"; import { customElement, state } from "lit/decorators.js"; ... private get isAdminRole(): boolean { - return this.playerRole === "admin" || this.playerRole === "root"; + return hasAdminRole(this.playerRole); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PlayerPanel.ts` around lines 81 - 83, Replace the hardcoded role check in the PlayerPanel getter "isAdminRole" with the shared helper "isAdminRole" used by the server: import the shared isAdminRole function into PlayerPanel and delegate the getter to call it with this.playerRole (e.g., return isAdminRole(this.playerRole)); remove the duplicated string checks so the panel stays in sync with the canonical permission logic.tests/server/KickPlayerAuthorization.test.ts (1)
89-92: Return to real timers inafterEach.
vi.clearAllTimers()removes scheduled work, but it does not leave fake-timer mode. Withoutvi.useRealTimers(), later tests in the same worker can inherit mocked time unexpectedly.🧪 Proposed fix
afterEach(() => { vi.restoreAllMocks(); vi.clearAllTimers(); + vi.useRealTimers(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server/KickPlayerAuthorization.test.ts` around lines 89 - 92, The afterEach hook currently calls vi.restoreAllMocks() and vi.clearAllTimers() but doesn't exit fake-timer mode; update the afterEach block (the callback that calls vi.restoreAllMocks and vi.clearAllTimers) to also call vi.useRealTimers() so tests don't retain mocked timers between runs—i.e., add vi.useRealTimers() alongside the existing vi.restoreAllMocks() and vi.clearAllTimers() calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/GameRenderer.ts`:
- Around line 208-212: Clear the previous role on the player panel before the
async auth call and also handle the false/rejection paths: call
playerPanel.setRole(null) immediately before invoking userAuth(), and ensure the
promise chain (the .then and .catch or an async/try-catch around userAuth())
clears the role (playerPanel.setRole(null)) when auth === false or an error is
thrown, and only sets playerPanel.setRole(auth.claims.role ?? null) when auth is
valid.
---
Nitpick comments:
In `@src/client/graphics/layers/PlayerPanel.ts`:
- Around line 81-83: Replace the hardcoded role check in the PlayerPanel getter
"isAdminRole" with the shared helper "isAdminRole" used by the server: import
the shared isAdminRole function into PlayerPanel and delegate the getter to call
it with this.playerRole (e.g., return isAdminRole(this.playerRole)); remove the
duplicated string checks so the panel stays in sync with the canonical
permission logic.
In `@tests/server/KickPlayerAuthorization.test.ts`:
- Around line 89-92: The afterEach hook currently calls vi.restoreAllMocks() and
vi.clearAllTimers() but doesn't exit fake-timer mode; update the afterEach block
(the callback that calls vi.restoreAllMocks and vi.clearAllTimers) to also call
vi.useRealTimers() so tests don't retain mocked timers between runs—i.e., add
vi.useRealTimers() alongside the existing vi.restoreAllMocks() and
vi.clearAllTimers() calls.
🪄 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: a304e03b-6b5a-485f-b918-c69682b4df42
📒 Files selected for processing (10)
resources/lang/en.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/PlayerModerationModal.tssrc/client/graphics/layers/PlayerPanel.tssrc/core/ApiSchemas.tssrc/server/Client.tssrc/server/GameServer.tssrc/server/Worker.tstests/client/graphics/layers/PlayerPanelKick.test.tstests/server/KickPlayerAuthorization.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- resources/lang/en.json
- src/server/Worker.ts
- src/server/Client.ts
- tests/client/graphics/layers/PlayerPanelKick.test.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
Author
done |
evanpelle
reviewed
Apr 17, 2026
Comment on lines
+208
to
+212
| userAuth().then((auth) => { | ||
| if (auth !== false) { | ||
| playerPanel.setRole(auth.claims.role ?? null); | ||
| } | ||
| }); |
Collaborator
There was a problem hiding this comment.
let's pass the role into GameRunner from LobbyConfig. I don't think layers should be calling auth
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/ApiSchemas.ts (1)
54-57: TieisAdminRoletoADMIN_ROLESso they cannot drift.Right now
ADMIN_ROLESlists the admin roles, butisAdminRolehardcodes the same two strings again. If someone later adds a role toADMIN_ROLES(say"mod"), the helper silently keeps the old behavior. Please derive one from the other so there is a single source of truth.🧹 Suggested tidy-up
-export const ADMIN_ROLES = ["admin", "root"] as const; -export function isAdminRole(role: string | null | undefined): boolean { - return role === "admin" || role === "root"; -} +export const ADMIN_ROLES = ["admin", "root"] as const; +export type AdminRole = (typeof ADMIN_ROLES)[number]; +export function isAdminRole( + role: string | null | undefined, +): role is AdminRole { + return role !== null && role !== undefined + && (ADMIN_ROLES as readonly string[]).includes(role); +}Bonus: the type guard lets callers narrow
roletoAdminRoleafter the check, which is friendlier than a plainboolean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ApiSchemas.ts` around lines 54 - 57, Update isAdminRole to derive its logic from the ADMIN_ROLES constant (instead of hardcoding strings) so there's a single source of truth: check membership by comparing the input role against ADMIN_ROLES (e.g., using includes or a Set) and change the signature to a type guard (role is AdminRole) so callers can narrow the type; refer to ADMIN_ROLES and isAdminRole (and add an AdminRole union type if not present) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/ApiSchemas.ts`:
- Around line 54-57: Update isAdminRole to derive its logic from the ADMIN_ROLES
constant (instead of hardcoding strings) so there's a single source of truth:
check membership by comparing the input role against ADMIN_ROLES (e.g., using
includes or a Set) and change the signature to a type guard (role is AdminRole)
so callers can narrow the type; refer to ADMIN_ROLES and isAdminRole (and add an
AdminRole union type if not present) when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ed94f71-0d92-4cfb-a104-763ff2d35a50
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/core/ApiSchemas.tssrc/server/GameServer.tssrc/server/Worker.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/Worker.ts
- src/server/GameServer.ts
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
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.
Description:
Players with the
adminflare can now kick players from any game (including public lobbies), not just the lobby creator in private lobbies.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n