Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds support for quick chat messages that reference two players. Adds a new "teaming_with" warning and translations, threads an optional second target ( Changes
Sequence DiagramsequenceDiagram
autonumber
participant User as User
participant ChatModal as ChatModal
participant Transport as Transport
participant ExecMgr as ExecutionManager
participant QuickExec as QuickChatExecution
participant Game as GameImpl
participant EventsDisplay as EventsDisplay
User->>ChatModal: select "teaming_with" phrase
ChatModal->>ChatModal: requiresPlayer2Selection = true
User->>ChatModal: pick Player 1
ChatModal->>ChatModal: set selectedPlayer
User->>ChatModal: pick Player 2
ChatModal->>ChatModal: set selectedPlayer2
User->>ChatModal: click Send
ChatModal->>Transport: SendQuickChatEvent(recipient, key, target, target2)
Transport->>ExecMgr: emit quick_chat intent (target, target2)
ExecMgr->>QuickExec: new QuickChatExecution(..., target, target2)
QuickExec->>Game: displayChat(message, category, target, target2, ...)
Game->>EventsDisplay: DisplayChatEvent (key, category, target, target2, ...)
EventsDisplay->>EventsDisplay: resolve [P1] and [P2]
EventsDisplay->>User: show "[Player1] is teaming with [Player2]!"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/ChatModal.ts (1)
54-63: 🛠️ Refactor suggestion | 🟠 MajorThis instance property
quickChatPhrasesis dead code — shadows the module-level import.
getPhrasesForCategoryat line 75 uses the module-levelquickChatPhrases(line 21), notthis.quickChatPhrases. This instance property is never referenced and can be removed.Suggested fix
- quickChatPhrases: Record< - string, - Array<{ text: string; requiresPlayer: boolean }> - > = { - help: [{ text: "Please give me troops!", requiresPlayer: false }], - attack: [{ text: "Attack [P1]!", requiresPlayer: true }], - defend: [{ text: "Defend [P1]!", requiresPlayer: true }], - greet: [{ text: "Hello!", requiresPlayer: false }], - misc: [{ text: "Let's go!", requiresPlayer: false }], - };
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/ChatModal.ts`:
- Around line 232-251: selectPlayer currently mixes translation keys and
rendered strings via previewText, which is fragile; change the flow so
previewText always holds the rendered display string (or maintain two explicit
fields like previewKey and previewText). Concretely: in selectPhrase (and
anywhere a phrase key is chosen) call translateText(key) once to get the base
translated string and store that result (or store key in previewKey and the
translated string in previewText), then in selectPlayer perform token
replacement on that translated string (replace "[P1]" / "[P2]") and set
previewText to the final rendered text; finally update render() to use
previewText directly (remove or skip translateText on previewText). Update the
selectPlayer/selectPhrase logic and any places referencing previewText to use
the new pattern so translation happens only once.
In `@src/client/graphics/layers/EventsDisplay.ts`:
- Around line 421-434: The fallback for resolving the second player is
incorrect: in the block handling event.target2 you call
this.game.player(event.target2) and compute target2Name =
target2Player?.displayName() ?? event.target but it should fall back to
event.target2 (the raw second-player ID). Update the fallback used when
target2Player?.displayName() is nullish so translatedMessage.replace("[P2]",
target2Name) uses event.target2 instead of event.target; keep the same try/catch
and warning behavior around the target2Player resolution.
🧹 Nitpick comments (5)
src/core/game/Game.ts (1)
813-821:target2parameter added todisplayChatinterface — works but the signature is getting long.Seven positional parameters with similar types (
string | undefined,PlayerID | null,boolean,string) make call sites hard to read. Consider grouping into an options object in a future refactor.src/client/graphics/layers/ChatModal.ts (4)
180-182: Send button is not disabled whenrequiresPlayer2Selectionis true but P2 selection hasn't started yet.Consider the scenario: a phrase that requires both players is selected, but only P1 has been chosen and the user somehow avoids triggering
selectingPlayer2(e.g., a race condition or future refactor changes the flow). The current check only guards onselectingPlayer2 && !selectedPlayer2, not onrequiresPlayer2Selection && !selectedPlayer2.A safer guard would be:
Suggested fix
?disabled=${!this.previewText || - (this.requiresPlayerSelection && !this.selectedPlayer) || - (this.selectingPlayer2 && !this.selectedPlayer2)} + (this.requiresPlayerSelection && !this.selectedPlayer) || + (this.requiresPlayer2Selection && !this.selectedPlayer2)}
253-257: Remove debugconsole.logcalls before merging.These log statements (lines 254-257) look like development artifacts. Consider removing or gating them behind a debug flag.
235-242: The?? nullon line 237 is unnecessary.
String.prototype.replace()always returns a string, so the nullish coalescing is a no-op here. Same applies to line 246.Suggested fix
- this.previewText = - this.previewText.replace("[P2]", player.name()) ?? null; + this.previewText = this.previewText.replace("[P2]", player.name());
304-317:open()does not resetselectedPlayerorselectedPlayer2, risking stale state.If a user opens the modal, selects players, closes without sending, and re-opens,
selectedPlayerandselectedPlayer2retain their previous values while the UI resets categories/phrases (viaclose()). The stale player references could point to players no longer alive or relevant. Consider resettingselectedPlayerinclose()as well.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/ChatModal.ts`:
- Around line 221-238: When re-selecting Player 1 in selectPlayer, clear any
stale Player 2 state so the UI and send logic don't use an old value: inside
selectPlayer (the branch that resets previewText from selectedPhraseTemplate and
sets selectedPlayer, selectingPlayer2, and requiresPlayerSelection), also set
selectedPlayer2 to null/undefined (and if needed clear any related flags that
gate the send button like requiresPlayer2Selection) so the previewText shows the
unresolved "[P2]" and the send button correctly disables until a new P2 is
chosen.
🧹 Nitpick comments (1)
src/client/graphics/layers/ChatModal.ts (1)
198-214: Three identicaltranslateTextcalls — store the result once.
selectedPhraseTemplate,selectedPhraseText, andpreviewTextall receive the same translated string. CalltranslateTextonce and reuse it.Proposed simplification
private selectPhrase(phrase: QuickChatPhrase) { this.selectedQuickChatKey = this.getFullQuickChatKey( this.selectedCategory!, phrase.key, ); - this.selectedPhraseTemplate = translateText( - `chat.${this.selectedCategory}.${phrase.key}`, - ); - this.selectedPhraseText = translateText( - `chat.${this.selectedCategory}.${phrase.key}`, - ); - this.previewText = translateText( - `chat.${this.selectedCategory}.${phrase.key}`, - ); + const translated = translateText( + `chat.${this.selectedCategory}.${phrase.key}`, + ); + this.selectedPhraseTemplate = translated; + this.selectedPhraseText = translated; + this.previewText = translated; this.requiresPlayerSelection = phrase.requiresPlayer; this.requiresPlayer2Selection = phrase.requiresPlayer2 ?? false; this.requestUpdate(); }
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/client/graphics/layers/ChatModal.ts (1)
198-215:⚠️ Potential issue | 🟠 MajorBug:
selectPhrasedoes not reset second-player state.If a user is mid-way through the P2 selection flow (so
selectingPlayer2 === true) and then clicks a different phrase,selectingPlayer2staystrue. This causes the player column header to still display "Player [P2]" for a phrase that may not need two players. Also,selectedPlayer2is never cleared here, so switching from a two-player phrase to a one-player phrase and sending will still include the staleselectedPlayer2id in the event payload.
selectCategoryalready resets all P2 fields —selectPhraseshould do the same.Proposed fix — reset P2 state in selectPhrase
private selectPhrase(phrase: QuickChatPhrase) { this.selectedQuickChatKey = this.getFullQuickChatKey( this.selectedCategory!, phrase.key, ); this.selectedPhraseTemplate = translateText( `chat.${this.selectedCategory}.${phrase.key}`, ); this.selectedPhraseText = translateText( `chat.${this.selectedCategory}.${phrase.key}`, ); this.previewText = translateText( `chat.${this.selectedCategory}.${phrase.key}`, ); this.requiresPlayerSelection = phrase.requiresPlayer; this.requiresPlayer2Selection = phrase.requiresPlayer2 ?? false; + this.selectedPlayer = null; + this.selectedPlayer2 = null; + this.selectingPlayer2 = false; this.requestUpdate(); }
🧹 Nitpick comments (1)
src/client/graphics/layers/ChatModal.ts (1)
243-246: Remove debugconsole.logcalls before merging.These four log statements (
"Sent message","Sender","Recipient","Key") look like development leftovers. Same for the logs inopen()(lines 295-296). They leak player info to the browser console and add noise.Proposed cleanup
private sendChatMessage() { - console.log("Sent message:", this.previewText); - console.log("Sender:", this.sender); - console.log("Recipient:", this.recipient); - console.log("Key:", this.selectedQuickChatKey); - if (this.sender && this.recipient && this.selectedQuickChatKey) {public open(sender?: PlayerView, recipient?: PlayerView) { if (sender && recipient) { - console.log("Sent message:", recipient); - console.log("Sent message:", sender); this.players = this.g
Caleb-Cohen
force-pushed
the
teaming-callout
branch
from
cb2e4da to
9eb33de
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/client/graphics/layers/ChatModal.ts (2)
245-250: Remove debugconsole.logcalls from the send path.These four log statements expose
previewText,sender,recipient, andselectedQuickChatKeyon every sent message. They are debug artifacts and should be removed before merging.♻️ Proposed clean-up
private sendChatMessage() { - console.log("Sent message:", this.previewText); - console.log("Sender:", this.sender); - console.log("Recipient:", this.recipient); - console.log("Key:", this.selectedQuickChatKey); - if (this.sender && this.recipient && this.selectedQuickChatKey) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ChatModal.ts` around lines 245 - 250, In the ChatModal class remove the debug console.log statements inside the sendChatMessage method (the lines logging this.previewText, this.sender, this.recipient, and this.selectedQuickChatKey); simply delete those console.log calls from sendChatMessage (or replace them with a proper, non-sensitive logger call if required) so sensitive message/send-path details are not logged.
198-218: Three identicaltranslateTextcalls can be collapsed to one.
selectedPhraseTemplate,selectedPhraseText, andpreviewTextare all set to the sametranslateText(...)result. Each call performs a DOM query and format lookup.♻️ Proposed clean-up
private selectPhrase(phrase: QuickChatPhrase) { this.selectedQuickChatKey = this.getFullQuickChatKey( this.selectedCategory!, phrase.key, ); - this.selectedPhraseTemplate = translateText( - `chat.${this.selectedCategory}.${phrase.key}`, - ); - this.selectedPhraseText = translateText( - `chat.${this.selectedCategory}.${phrase.key}`, - ); - this.previewText = translateText( - `chat.${this.selectedCategory}.${phrase.key}`, - ); + const translatedPhrase = translateText( + `chat.${this.selectedCategory}.${phrase.key}`, + ); + this.selectedPhraseTemplate = translatedPhrase; + this.selectedPhraseText = translatedPhrase; + this.previewText = translatedPhrase; this.requiresPlayerSelection = phrase.requiresPlayer;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ChatModal.ts` around lines 198 - 218, In selectPhrase, avoid calling translateText three times; compute const translated = translateText(`chat.${this.selectedCategory}.${phrase.key}`) once and assign translated to this.selectedPhraseTemplate, this.selectedPhraseText, and this.previewText; keep the rest of the method (requiresPlayer*, selectedPlayer*, selectUpdate call) unchanged and call this.requestUpdate() at the end.tests/QuickChatExecutions.test.ts (1)
52-52:?? []guards onDisplayChatEventarrays are unnecessary.
createGameUpdatesMap()pre-initialises everyGameUpdateTypekey to an empty array, soupdates[GameUpdateType.DisplayChatEvent]is always an array. The same pattern applies on lines 89, 111, 132, 135, 174, and 196.♻️ Example clean-up (apply similarly to all occurrences)
- const chatEvents = updates[GameUpdateType.DisplayChatEvent] ?? []; + const chatEvents = updates[GameUpdateType.DisplayChatEvent];Based on learnings:
game.executeNextTick()returns thecreateGameUpdatesMap()result, which initialises every key to[]; optional-chaining and nullish-coalescing are not needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/QuickChatExecutions.test.ts` at line 52, The guard "?? []" and any optional-chaining on accesses like updates[GameUpdateType.DisplayChatEvent] are unnecessary because createGameUpdatesMap() (the structure returned by game.executeNextTick()) pre-initialises every GameUpdateType key to an array; remove the nullish-coalescing and optional-chaining so lines referencing updates[GameUpdateType.DisplayChatEvent] (and the other occurrences on lines with GameUpdateType keys) directly use the array returned from game.executeNextTick() (locate the references in tests/QuickChatExecutions.test.ts and update the relevant expressions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/QuickChatExecutions.test.ts`:
- Around line 39-74: The test is passing the i18n path
"chat.warnings.teaming_with" to QuickChatExecution but
ChatModal.getFullQuickChatKey() expects the compact chat key
"warnings.teaming_with"; update the QuickChatExecution instantiation in the test
to use "warnings.teaming_with" (replace the three-segment string) so
category/key parsing matches production behavior and the assertions for category
and key (via QuickChatExecution handling) will be correct.
---
Nitpick comments:
In `@src/client/graphics/layers/ChatModal.ts`:
- Around line 245-250: In the ChatModal class remove the debug console.log
statements inside the sendChatMessage method (the lines logging
this.previewText, this.sender, this.recipient, and this.selectedQuickChatKey);
simply delete those console.log calls from sendChatMessage (or replace them with
a proper, non-sensitive logger call if required) so sensitive message/send-path
details are not logged.
- Around line 198-218: In selectPhrase, avoid calling translateText three times;
compute const translated =
translateText(`chat.${this.selectedCategory}.${phrase.key}`) once and assign
translated to this.selectedPhraseTemplate, this.selectedPhraseText, and
this.previewText; keep the rest of the method (requiresPlayer*, selectedPlayer*,
selectUpdate call) unchanged and call this.requestUpdate() at the end.
In `@tests/QuickChatExecutions.test.ts`:
- Line 52: The guard "?? []" and any optional-chaining on accesses like
updates[GameUpdateType.DisplayChatEvent] are unnecessary because
createGameUpdatesMap() (the structure returned by game.executeNextTick())
pre-initialises every GameUpdateType key to an array; remove the
nullish-coalescing and optional-chaining so lines referencing
updates[GameUpdateType.DisplayChatEvent] (and the other occurrences on lines
with GameUpdateType keys) directly use the array returned from
game.executeNextTick() (locate the references in
tests/QuickChatExecutions.test.ts and update the relevant expressions).
d6c8056 to
21c5077
Compare
…l translateText for them.
….replace() and updated dsiabled guard to requiresPlayer2Selection instead of selectingPlayer2.
…as it was dead code.
… of re-translating on every render
… the teaming_with quick chat command.
…ated instead of calling translateText three times within selectPhrase function. Remove console.log debuggin within sendChatMessage().
…i18n translation path. Deleted redundant tests.
Caleb-Cohen
force-pushed
the
teaming-callout
branch
from
21c5077 to
f03a7fe
Compare
3 participants
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #3189
Description:
Adds a new "teaming with" quick chat phrase that allows players to call out two players who are teaming, using a two-player selection flow in the chat modal.
Screenshots
Modal before countries are selected
Modal after countries are selected
Sent quick chat message
Describe the PR.
teaming_withentry toQuickChat.jsonwithrequiresPlayerandrequiresPlayer2flagsTeaming_with: "[P1] is teaming with [P2]!",player_1, andplayer_2to en.jsontarget2field toQuickChatIntentschema,SendQuickChatEvent,QuickChatExecution,displayChatMessage, andDisplayChatMessageUpdateChatModalto support a two-step player selection. After picking P1, the modal transitions to P2 mode with P1 disabled on the list. Selection then loops back to P1 so players can correct their selection or submit.EventsDisplayto resolve the[P2]placeholder usingtarget2Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
CalebCohen