Lobby Creators can kick players in game #2687
Conversation
WalkthroughServer now includes Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(245,250,255)
participant ClientUI as Client UI
participant ClientRunner as ClientGameRunner
participant GameRunner as Server/GameRunner
participant GameView as GameView
participant Server as GameServer
end
Note over Server,GameRunner: Game start with lobbyCreatorID
Server->>GameRunner: start(gameStart{..., lobbyCreatorID})
GameRunner->>GameView: new(game, players, lobbyCreatorID)
GameView->>GameView: mark lobbyCreator (compare IDs)
GameRunner->>ClientRunner: push GameUpdate (includes per-player canKick)
ClientRunner->>ClientUI: forward update
ClientUI->>ClientUI: render PlayerPanel (show Kick if canKick)
ClientUI->>Server: SendKickPlayerIntent(targetPlayerID)
Server->>GameRunner: validate kick -> apply -> push updated GameUpdate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-06-02T14:27:37.609ZApplied to files:
📚 Learning: 2025-10-08T17:14:49.369ZApplied to files:
🧬 Code graph analysis (1)src/core/GameRunner.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
318-348: Critical: Validate that requester is the claimed lobby creator.The
creatorClientIDfrom the API request is only validated for format usingID.safeParse()at Worker.ts:116, but there is no authorization check that the client making the request is actually that creator. Any client can sendcreatorClientID=<victim_id>and successfully create a game with the victim as creator.The endpoint needs to verify the requester's identity matches
creatorClientIDbefore accepting it. Use theverifyClientTokenfunction (already imported) or similar JWT validation to ensure the request's authenticated user equals the claimed creator.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
resources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/graphics/layers/PlayerPanel.tssrc/core/GameRunner.tssrc/core/Schemas.tssrc/core/game/Game.tssrc/core/game/GameView.tssrc/server/GameServer.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/GameRunner.tssrc/client/ClientGameRunner.tssrc/core/game/GameView.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/server/GameServer.tssrc/core/Schemas.tssrc/core/game/Game.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/ClientGameRunner.tssrc/core/game/GameView.tssrc/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/ClientGameRunner.tssrc/core/game/GameView.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/game/GameView.ts
🧬 Code graph analysis (3)
src/core/GameRunner.ts (1)
src/core/Schemas.ts (1)
ClientID(26-26)
src/core/game/GameView.ts (1)
src/core/Schemas.ts (1)
ClientID(26-26)
src/client/graphics/layers/PlayerPanel.ts (4)
src/core/game/GameView.ts (1)
PlayerView(185-578)src/client/Transport.ts (1)
SendKickPlayerIntentEvent(173-175)src/client/components/ui/ActionButton.ts (1)
actionButton(43-69)src/client/Utils.ts (1)
translateText(92-147)
🔇 Additional comments (6)
src/core/game/Game.ts (1)
799-799: LGTM - Clean addition following established patterns.The optional
canKickfield follows the same pattern as existing flags likecanDonateGoldandcanDonateTroops. Based on learnings, PlayerInteraction changes don't require schema updates.src/client/ClientGameRunner.ts (1)
194-194: LGTM - Safe parameter forwarding.The
lobbyCreatorIDis safely accessed after thegameStartInfoundefined check at line 163. The parameter is correctly passed to the updated GameView constructor.resources/lang/en.json (1)
697-697: LGTM - Translation key follows conventions.The "kick" translation key is clearly named and properly placed within the
player_panelsection. The value "Kick Player" is concise and actionable.src/core/game/GameView.ts (1)
620-622: LGTM - Simple identity check.The
isLobbyCreatormethod performs a straightforward equality check. Client-side validation is intentionally minimal here, which is appropriate since the server is responsible for validating kick permissions (as seen in GameServer.ts lines 320-328).src/core/GameRunner.ts (1)
212-214: Logic is sound but depends on server validation.The
canKicklogic correctly:
- Checks if the querying player is the lobby creator
- Prevents self-kick with
player.clientID() !== other.clientID()- Handles null clientID safely (bots/nations can't kick and can't be kicked)
However, this depends on
lobbyCreatorIDbeing securely validated server-side. See my comment on GameServer.ts regarding verification of lobby creator authentication.src/server/GameServer.ts (1)
465-465: Schema validation correctly handles undefined lobbyCreatorID.The
GameStartInfoSchemadefineslobbyCreatorID: ID.optional()and the code properly passes the optional field usingsafeParse()with error handling. Validation will succeed whenlobbyCreatorIDis undefined (public games), and the TypeScript type system correctly infers it as an optional property.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)
5-5: Consider a more intuitive icon for the kick action.The
disabledIconname doesn't semantically match the kick action. Users might expect a boot, exit, or ban icon instead. While functional, a more descriptive icon would improve clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerPanel.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (3)
src/core/game/GameView.ts (1)
PlayerView(185-578)src/client/Transport.ts (1)
SendKickPlayerIntentEvent(173-175)src/client/components/ui/ActionButton.ts (1)
actionButton(43-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (3)
src/client/graphics/layers/PlayerPanel.ts (3)
279-288: LGTM! Defensive null check properly added.The method now includes a defensive guard against null
clientID, addressing the previous concern about potential runtime errors with bot/nation players. The implementation follows the established pattern of other action handlers.
634-634: LGTM! Consistent permission check pattern.The
canKickflag extraction follows the same pattern as other permission flags in this method, using optional chaining for safe access.
735-744: The translation keyplayer_panel.kickalready exists in en.json (line 697) with the value "Kick Player". The code correctly references this existing translation.
|
@Restart2008 how does this affect the allow all trade button if you cancel all trade? Are you able to resume trading if you cancel it with everyone? |
From what I saw during testing, the kick button shouldn't be affecting the allow all trade button at all. I think that was what you were asking? I added some updated screenshots. |
|
What if the lobby creator accidentally clicks that button? |
If you open your player panel, you should see two buttons: “Stop trading with all” and “Start trading with all.” Your screenshot shows something different. That said, can the lobby creator kick themselves? |
|
FYI, I have another PR #2657 where we also need to check if the player is the lobby creator during the game |
Thats a good point, maybe a confirm kick popup-button appears to make sure that the lobby creator doesn't accidentally click that button?
I actually didn't test that yesterday so I have no clue, I'll try testing it today. If I do see that lobby creator's can kick themselves then I'll add a fix for that. |
In the screenshot it shows the player panel for the other player (Anon 712) while I'm Anon 339. I remember from yesterday that the "stop trade with all" and "start trade with all" buttons for my own player panel are still present alongside the kick player button. I'll recheck this again today when I test it with another player. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)
735-744: Request confirmation: Is the accidental-kick prevention popup planned?The kick button renders correctly and the logic properly prevents kicking the lobby creator (including self-kick). However, in the PR discussion you mentioned adding a confirmation popup to prevent accidental clicks. This safety measure isn't present in the current code—the kick executes immediately when clicked.
Is the confirmation dialog:
- Planned for a follow-up commit/PR?
- Deferred for later?
- No longer needed?
For destructive actions like kicking, a confirmation step is a recommended UX pattern.
Optional: Icon naming clarity
Minor point:
disabledIconas the kick button icon might confuse readers. Consider renaming to something likekickIconorremovePlayerIconfor clearer intent, or document why "disabled" represents kicking.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerPanel.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (2)
src/client/Transport.ts (1)
SendKickPlayerIntentEvent(173-175)src/client/components/ui/ActionButton.ts (1)
actionButton(43-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)
279-288: Defensive guard implemented correctly.The null check on
targetClientIDproperly prevents crashes when the target player lacks a clientID (bots and nations). Clean fix that addresses the previous review concern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)
742-751: Kick button logic looks correct.The condition
canKick && !this.g.isLobbyCreator(other)correctly prevents:
- Non-lobby-creators from seeing the kick button (since
canKickwould be false)- The lobby creator from kicking themselves (since
isLobbyCreator(other)returns true when viewing their own panel)This addresses the self-kick concern raised in PR comments.
One minor note: using
disabledIconfor a "Kick" action is a bit semantically odd. If a more fitting icon becomes available (like a boot or door icon), consider swapping it later.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.jsonsrc/client/graphics/layers/PlayerPanel.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-07-30T19:46:23.797Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1436
File: src/server/Worker.ts:93-93
Timestamp: 2025-07-30T19:46:23.797Z
Learning: Input validation at API boundaries is critical for security. All client-provided data, especially parameters used for authorization decisions like creatorClientID in the kick player functionality, must be validated for type, format, length, and business logic constraints before processing.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (3)
src/client/Utils.ts (1)
translateText(92-147)src/client/Transport.ts (1)
SendKickPlayerIntentEvent(173-175)src/client/components/ui/ActionButton.ts (1)
actionButton(43-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (3)
src/client/graphics/layers/PlayerPanel.ts (2)
279-295: Well done on the defensive null check.The past review concern about
other.clientID()!causing runtime errors for bots/nations is now properly addressed. The confirmation dialog with translated text and the early return for null clientID make this robust.
5-5: Supporting imports and variable extraction look good.The new imports for
disabledIconandSendKickPlayerIntentEvent, along with thecanKickextraction fromthis.actions?.interaction, cleanly integrate with the existing code structure.Also applies to: 35-35, 641-641
resources/lang/en.json (1)
696-698: Translation keys properly added.The
kickandkick_confirmkeys are correctly placed underplayer_panelnamespace and use proper ICU placeholder syntax ({name}) that matches thetranslateTextcall inPlayerPanel.ts. Clear and simple wording.
|
@abdallahbahrawi1 I got some new updated screenshots. 1st screenshot shows the kick confirmation. 2nd screenshot shows the hosts player info panel. In that you see no kick player button to prevent lobby hosts from kicking themselves out of the game lol but the trading buttons are present. |
| canEmbargo: !player.hasEmbargoAgainst(other), | ||
| canKick: | ||
| this.lobbyCreatorID === player.clientID() && | ||
| player.clientID() !== other.clientID(), |
There was a problem hiding this comment.
we shhould also add a chec kthat other.playerType() == PlayerType.Human
| lobbyCreatedAt: z.number(), | ||
| config: GameConfigSchema, | ||
| players: PlayerSchema.array(), | ||
| lobbyCreatorID: ID.optional(), |
There was a problem hiding this comment.
this should be removed, PlayerScheam already has isLobbyCreator field
| cosmetics: c.cosmetics, | ||
| isLobbyCreator: this.lobbyCreatorID === c.clientID, | ||
| })), | ||
| lobbyCreatorID: this.lobbyCreatorID, |
| canDonateTroops: player.canDonateTroops(other), | ||
| canEmbargo: !player.hasEmbargoAgainst(other), | ||
| canKick: | ||
| this.lobbyCreatorID === player.clientID() && |
There was a problem hiding this comment.
do player.isLobbyCreator() instead
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #2686 Allow Lobby Creator to kick players in game
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME
Restart