feat: add Quick Chat radial submenu with configurable presets#3529
Open
baculinivan-web wants to merge 9 commits intoopenfrontio:mainfrom
Open
feat: add Quick Chat radial submenu with configurable presets#3529baculinivan-web wants to merge 9 commits intoopenfrontio:mainfrom
baculinivan-web wants to merge 9 commits intoopenfrontio:mainfrom
Conversation
- Repurposes the 'i' (Info) button in the right-click radial menu to open a Quick Chat submenu with configurable preset actions - Presets support quick-chat messages, emoji panel, and start/stop trade - Defaults: 'Give me troops', emoji panel, 'Attack [P1]!' - For actions requiring a target player (requiresPlayer: true), enters a target-selection mode: cursor changes to pointer, a badge follows the mouse, and clicking an enemy tile sends the message with that player as the target - Fixes attack-on-click bug during target selection by guarding ClientGameRunner and UnitLayer event handlers - Adds a permanent 'Player Info' button (original Info behaviour) and a 'Customize Presets' settings button to the submenu - QuickChatConfigModal uses the same chat-columns UI as ChatModal - Presets auto-save to localStorage; reset requires two-step confirmation - All strings go through translateText() with new quick_chat.* i18n keys - Adds .kiro/ to .gitignore - Fixes pre-existing localStorage mock issue in tests/setup.ts
|
|
Contributor
|
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 a Quick Chat system: preset persistence and editor, radial-menu integration with dynamic preset items, a target-selection mode and cursor badge, renderer wiring for new UI elements, input guards during selection, and a test localStorage mock. Changes
Sequence DiagramsequenceDiagram
actor Player
participant RadialMenu
participant QuickChatPresetService
participant TargetSelectionMode
participant TargetSelectionLayer
participant EventBus
participant Map
Player->>RadialMenu: Right-click -> open menu
RadialMenu->>QuickChatPresetService: load presets
RadialMenu->>Player: show quick-chat submenu
Player->>RadialMenu: Select preset (e.g., "Request Attack")
alt Phrase requires target
RadialMenu->>TargetSelectionMode: enter(phraseKey, recipient)
TargetSelectionMode->>TargetSelectionLayer: activate badge
TargetSelectionLayer->>Player: change cursor, show badge
Player->>Map: Click country
Map->>EventBus: MouseUp with world coords
EventBus->>RadialMenu: deliver click
RadialMenu->>Map: resolve coords -> owner
RadialMenu->>EventBus: emit SendQuickChatEvent(recipient, phraseKey, target)
RadialMenu->>TargetSelectionMode: exit()
TargetSelectionLayer->>TargetSelectionLayer: deactivate badge
else Phrase standalone (emoji/trade)
RadialMenu->>QuickChatPresetService: handle action immediately
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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
🧹 Nitpick comments (6)
src/client/graphics/layers/QuickChatConfigModal.ts (3)
53-69: Reset timeout not cleared on close.If the user clicks Reset (arming the 3-second confirmation), then closes the modal before 3 seconds, the
setTimeoutcallback will still fire and callthis.requestUpdate()on a closed modal.This won't crash (Lit handles it gracefully), but it's a minor leak. Store the timer ID and clear it in
close().Proposed fix
+ private _resetTimer: ReturnType<typeof setTimeout> | null = null; + private reset() { if (!this.confirmReset) { this.confirmReset = true; this.requestUpdate(); - setTimeout(() => { + this._resetTimer = setTimeout(() => { this.confirmReset = false; this.requestUpdate(); }, 3000); return; } + if (this._resetTimer) { + clearTimeout(this._resetTimer); + this._resetTimer = null; + } this.slots = [...DEFAULT_PRESETS]; // ... } close() { + if (this._resetTimer) { + clearTimeout(this._resetTimer); + this._resetTimer = null; + } this.editingIndex = null; // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/QuickChatConfigModal.ts` around lines 53 - 69, The reset() method arms a 3s confirmation using setTimeout but never stores or clears the timer, so if the modal is closed the callback still runs; add a property (e.g., this.resetTimerId) to store the timer ID when calling setTimeout in reset(), clear any existing timer before setting a new one, and clearTimeout(this.resetTimerId) in close() (and after the timeout callback runs) to prevent the callback from firing on a closed component; update references to confirmReset, requestUpdate(), and persist() accordingly so behavior remains the same.
11-11: DuplicateMAX_SLOTSconstant.
MAX_SLOTS = 5is also defined inQuickChatPresetService.ts. Consider importing from the service to keep the constraint in one place.Proposed fix
import { DEFAULT_PRESETS, + MAX_SLOTS, PresetSlot, QuickChatPresetService, } from "./QuickChatPresetService"; import { translateText } from "../../Utils"; - -const MAX_SLOTS = 5;You'll need to export
MAX_SLOTSfromQuickChatPresetService.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/QuickChatConfigModal.ts` at line 11, The constant MAX_SLOTS is duplicated in QuickChatConfigModal.ts; remove the local definition and import the single source of truth by exporting MAX_SLOTS from QuickChatPresetService (add an export there) and then import { MAX_SLOTS } into QuickChatConfigModal (replace the local const). Update any usage in QuickChatConfigModal to reference the imported MAX_SLOTS so the constraint is maintained in QuickChatPresetService only.
45-51: Silent error logging may hide validation failures.If
QuickChatPresetService.save()throws (e.g., slot count out of range), the error is logged but the user sees no feedback. The modal's guards (length <= 1,length >= MAX_SLOTS) should prevent this, but if they drift out of sync with the service'sMIN_SLOTS/MAX_SLOTS, failures will be silent.Since the guards are currently correct, this is low risk. Just be aware if you change the slot limits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/QuickChatConfigModal.ts` around lines 45 - 51, The persist() method currently catches errors from QuickChatPresetService.getInstance().save(this.slots) and only logs them to the console, which hides validation failures from users; update persist() to surface save errors to the user (e.g., show a modal error, toast, or call a UI error handler) and include the error details in the message so users know their slot changes failed—keep the console.error for debugging but also call the UI feedback mechanism used elsewhere in this component (or a centralized notifier) so failures in QuickChatConfigModal.persist are visible to the user.src/client/graphics/layers/MainRadialMenu.ts (1)
116-127: Event listeners are never unregistered.The
MouseUpEvent,TouchEvent, andCloseViewEventhandlers are registered ininit()but never removed. IfMainRadialMenuis ever destroyed and recreated, this could cause duplicate handlers.For a singleton-like component that lives for the entire game session, this is acceptable. Just be aware if the lifecycle changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/MainRadialMenu.ts` around lines 116 - 127, The event listeners for MouseUpEvent, TouchEvent and CloseViewEvent registered in init() are never removed; store the listener callbacks (the functions that call handleSelectClick and TargetSelectionMode.getInstance().exit) as properties on MainRadialMenu when you register them, and unregister them in a teardown/destroy method by calling the corresponding eventBus.off/unsubscribe API for those same callbacks so duplicate handlers aren't left behind if the component is recreated; update init() to assign the callbacks and add a destroy()/dispose() that removes MouseUpEvent, TouchEvent and CloseViewEvent handlers using the stored references.src/client/graphics/layers/QuickChatPresetService.ts (1)
19-20: Consider exportingMIN_SLOTSandMAX_SLOTS.
QuickChatConfigModal.tsduplicatesMAX_SLOTS. Exporting these constants centralizes the limits.Proposed fix
-const MIN_SLOTS = 1; -const MAX_SLOTS = 5; +export const MIN_SLOTS = 1; +export const MAX_SLOTS = 5;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/QuickChatPresetService.ts` around lines 19 - 20, Export the slot limit constants so other modules use the centralized values: change MIN_SLOTS and MAX_SLOTS in QuickChatPresetService.ts to exported constants (export const MIN_SLOTS, export const MAX_SLOTS) and then update QuickChatConfigModal.ts to import those constants instead of duplicating MAX_SLOTS; ensure any references to MIN_SLOTS/MAX_SLOTS use the imported names and remove the duplicate definition in QuickChatConfigModal.src/client/graphics/layers/TargetSelectionLayer.ts (1)
45-46: Direct manipulation ofdocument.body.style.cursor.This works but could conflict if other components also set body cursor. Consider using a CSS class on body instead (e.g.,
document.body.classList.add("target-selecting")) with a stylesheet rule. This makes it easier to debug and avoids style collisions.That said, for this specific use case where the mode is short-lived and modal, direct style manipulation is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TargetSelectionLayer.ts` around lines 45 - 46, The code directly sets document.body.style.cursor in TargetSelectionLayer (near the call to this.requestUpdate()), which can conflict with other components; change it to toggle a body CSS class instead (e.g., document.body.classList.add("target-selecting") and .remove("target-selecting")) and add a corresponding CSS rule (.target-selecting { cursor: pointer !important; }) in the stylesheet; ensure you update both the entry point where the cursor is set and the cleanup/exit path so the class is always removed (check the same methods that call requestUpdate and any abort/teardown logic in TargetSelectionLayer).
🤖 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 232-249: The file GameRenderer.ts has formatting issues flagged by
Prettier; run Prettier (or your project's format script) on
src/client/graphics/GameRenderer.ts to fix whitespace/comma/line-breaks around
the targetSelectionLayer and quickChatConfigModal block (the code that queries
"target-selection-layer" and "quick-chat-config-modal" and sets eventBus, game,
transformHandler), then re-stage the formatted file and push the commit so CI
style checks pass.
- Around line 232-241: The code obtains targetSelectionLayer (variable
targetSelectionLayer) and logs when it's not an instance of TargetSelectionLayer
but still proceeds to add it to the renderer's layers, which can cause runtime
crashes; update the logic in GameRenderer (around the code that queries
"target-selection-layer" and the other similar occurrence) to only assign
eventBus/game/transformHandler and push the element into the layers array when
targetSelectionLayer is an instance of TargetSelectionLayer—otherwise skip
adding it (or return/throw) so render/tick loops never receive a null/invalid
layer.
In `@src/client/graphics/layers/QuickChatConfigModal.ts`:
- Around line 249-251: Replace the hardcoded English string "[needs target]" in
QuickChatConfigModal's render block (where phrase.requiresPlayer is checked)
with a call to the translation helper (e.g., translateText("needs_target")), and
add the "needs_target" key to the locale files (start with en.json ->
"needs_target": "[needs target]") so the UI text is localized consistently with
other strings.
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 501-511: The tooltip for playerInfoItem is using a hardcoded
string; replace tooltipItems: [{ text: "Player Info", className: "title" }] with
a localized call such as tooltipItems: [{ text:
translateText("quick_chat.player_info"), className: "title" }], ensure
translateText is imported or available in this module (or use the existing i18n
helper used elsewhere), and keep the rest of playerInfoItem (id/name/action)
unchanged so the tooltip displays the localized "player_info" key.
---
Nitpick comments:
In `@src/client/graphics/layers/MainRadialMenu.ts`:
- Around line 116-127: The event listeners for MouseUpEvent, TouchEvent and
CloseViewEvent registered in init() are never removed; store the listener
callbacks (the functions that call handleSelectClick and
TargetSelectionMode.getInstance().exit) as properties on MainRadialMenu when you
register them, and unregister them in a teardown/destroy method by calling the
corresponding eventBus.off/unsubscribe API for those same callbacks so duplicate
handlers aren't left behind if the component is recreated; update init() to
assign the callbacks and add a destroy()/dispose() that removes MouseUpEvent,
TouchEvent and CloseViewEvent handlers using the stored references.
In `@src/client/graphics/layers/QuickChatConfigModal.ts`:
- Around line 53-69: The reset() method arms a 3s confirmation using setTimeout
but never stores or clears the timer, so if the modal is closed the callback
still runs; add a property (e.g., this.resetTimerId) to store the timer ID when
calling setTimeout in reset(), clear any existing timer before setting a new
one, and clearTimeout(this.resetTimerId) in close() (and after the timeout
callback runs) to prevent the callback from firing on a closed component; update
references to confirmReset, requestUpdate(), and persist() accordingly so
behavior remains the same.
- Line 11: The constant MAX_SLOTS is duplicated in QuickChatConfigModal.ts;
remove the local definition and import the single source of truth by exporting
MAX_SLOTS from QuickChatPresetService (add an export there) and then import {
MAX_SLOTS } into QuickChatConfigModal (replace the local const). Update any
usage in QuickChatConfigModal to reference the imported MAX_SLOTS so the
constraint is maintained in QuickChatPresetService only.
- Around line 45-51: The persist() method currently catches errors from
QuickChatPresetService.getInstance().save(this.slots) and only logs them to the
console, which hides validation failures from users; update persist() to surface
save errors to the user (e.g., show a modal error, toast, or call a UI error
handler) and include the error details in the message so users know their slot
changes failed—keep the console.error for debugging but also call the UI
feedback mechanism used elsewhere in this component (or a centralized notifier)
so failures in QuickChatConfigModal.persist are visible to the user.
In `@src/client/graphics/layers/QuickChatPresetService.ts`:
- Around line 19-20: Export the slot limit constants so other modules use the
centralized values: change MIN_SLOTS and MAX_SLOTS in QuickChatPresetService.ts
to exported constants (export const MIN_SLOTS, export const MAX_SLOTS) and then
update QuickChatConfigModal.ts to import those constants instead of duplicating
MAX_SLOTS; ensure any references to MIN_SLOTS/MAX_SLOTS use the imported names
and remove the duplicate definition in QuickChatConfigModal.
In `@src/client/graphics/layers/TargetSelectionLayer.ts`:
- Around line 45-46: The code directly sets document.body.style.cursor in
TargetSelectionLayer (near the call to this.requestUpdate()), which can conflict
with other components; change it to toggle a body CSS class instead (e.g.,
document.body.classList.add("target-selecting") and .remove("target-selecting"))
and add a corresponding CSS rule (.target-selecting { cursor: pointer
!important; }) in the stylesheet; ensure you update both the entry point where
the cursor is set and the cleanup/exit path so the class is always removed
(check the same methods that call requestUpdate and any abort/teardown logic in
TargetSelectionLayer).
🪄 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: ab701cde-aa7d-420e-b1e9-4222370ce9ea
📒 Files selected for processing (15)
.gitignoreindex.htmlresources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/InputHandler.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/QuickChatConfigModal.tssrc/client/graphics/layers/QuickChatPresetService.tssrc/client/graphics/layers/RadialMenu.tssrc/client/graphics/layers/RadialMenuElements.tssrc/client/graphics/layers/TargetSelectionLayer.tssrc/client/graphics/layers/TargetSelectionMode.tssrc/client/graphics/layers/UnitLayer.tstests/setup.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
baculinivan-web
force-pushed
the
feature/quick-chat-radial-menu
branch
from
9d4419f to
55b6689
Compare
- Add targetSelectionLayer to layers array conditionally to avoid runtime crash - Replace hardcoded 'Player Info' tooltip with translateText() - Replace hardcoded '[needs target]' with translateText() - Add quick_chat.player_info and quick_chat.needs_target i18n keys
Contributor
Author
|
@coderabbitai resolve |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/client/graphics/layers/RadialMenuElements.ts (1)
499-505:⚠️ Potential issue | 🟡 MinorLocalize the “Player Info” tooltip.
This tooltip is still hardcoded English, unlike the rest of the quick-chat submenu.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/RadialMenuElements.ts` around lines 499 - 505, The "Player Info" tooltip in the playerInfoItem object is hardcoded; replace that literal with the project's localization call (the same helper used elsewhere, e.g., t(...) or i18n.translate(...)) so tooltipItems for playerInfoItem uses a localized string key (like "radial.playerInfo" or the existing i18n key pattern) instead of "Player Info"; update the tooltipItems entry inside playerInfoItem to call the localization function and pass the resulting string as text.src/client/graphics/layers/QuickChatConfigModal.ts (1)
278-282:⚠️ Potential issue | 🟡 MinorLocalize the “[needs target]” hint.
This is still hardcoded English inside the modal. Please move it to
translateText(...)like the other visible strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/QuickChatConfigModal.ts` around lines 278 - 282, The hardcoded hint "[needs target]" inside QuickChatConfigModal should be localized: replace the literal string rendered when phrase.requiresPlayer with a call to translateText (e.g., translateText("needs_target") or the appropriate key used elsewhere) while preserving the existing span styling and placement; if the translation key doesn’t exist, add a new key "needs_target" (or agreed key) to the localization resources and use translateText(...) in the conditional render where phrase.requiresPlayer is checked.
🤖 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/layers/QuickChatConfigModal.ts`:
- Around line 181-190: The remove button in QuickChatConfigModal's render (the
chat-option-button that currently displays only "✕") lacks an accessible name;
update the button produced in the render of QuickChatConfigModal to include a
translated aria-label and title (e.g. using the existing i18n/translation helper
like t or translate) that includes the slot index/number so screen readers know
which preset will be removed, and keep the existing click handler calling
this.removeSlot(i); ensure both aria-label and title are set consistently for
accessibility.
- Around line 53-60: The reset confirmation timeout in QuickChatConfigModal
currently doesn't clear prior timers, causing stale callbacks to flip
confirmReset unexpectedly; update the reset() method to clear any existing
resetConfirmTimer before starting a new setTimeout and store the timer id in
resetConfirmTimer, and also clear/reset resetConfirmTimer inside the open() and
close() methods so reopening the modal can't be affected by a previous timeout
(references: reset(), confirmReset, resetConfirmTimer, open(), close()).
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 490-491: The subMenu function currently reads params.selected
without guarding against params being undefined; update the guard in subMenu to
explicitly check params first (e.g., if (params === undefined || params.selected
== null) return [];), so it returns early when params is missing or selected is
null/undefined and avoids the spawn-phase crash in subMenu.
- Around line 439-462: The trade preset currently only checks p.selected ===
null and can call p.playerActionHandler.handleEmbargo on your own player; update
the preset's disabled and action logic to block self and reuse the interaction
eligibility instead: in the element returned for slot.type === "trade" adjust
disabled to also return true when p.selected refers to the local/self player
(i.e. when selecting your own player/territory) and/or when the standard trade
eligibility check on p.playerActions?.interaction (the same checks the normal
trade UI uses, e.g. canEmbargo or any canToggleTrade method) fails; in the
action handler before calling p.playerActionHandler.handleEmbargo, verify
p.selected is not self and that the interaction eligibility allows toggling
trade, and only then call handleEmbargo(p.selected!, ...).
---
Duplicate comments:
In `@src/client/graphics/layers/QuickChatConfigModal.ts`:
- Around line 278-282: The hardcoded hint "[needs target]" inside
QuickChatConfigModal should be localized: replace the literal string rendered
when phrase.requiresPlayer with a call to translateText (e.g.,
translateText("needs_target") or the appropriate key used elsewhere) while
preserving the existing span styling and placement; if the translation key
doesn’t exist, add a new key "needs_target" (or agreed key) to the localization
resources and use translateText(...) in the conditional render where
phrase.requiresPlayer is checked.
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 499-505: The "Player Info" tooltip in the playerInfoItem object is
hardcoded; replace that literal with the project's localization call (the same
helper used elsewhere, e.g., t(...) or i18n.translate(...)) so tooltipItems for
playerInfoItem uses a localized string key (like "radial.playerInfo" or the
existing i18n key pattern) instead of "Player Info"; update the tooltipItems
entry inside playerInfoItem to call the localization function and pass the
resulting string as text.
🪄 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: bd6768dd-305c-4897-962e-fc41e7e33358
📒 Files selected for processing (4)
src/client/InputHandler.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/QuickChatConfigModal.tssrc/client/graphics/layers/RadialMenuElements.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/InputHandler.ts
- src/client/graphics/GameRenderer.ts
baculinivan-web
force-pushed
the
feature/quick-chat-radial-menu
branch
from
0e8c9e8 to
cdeb662
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/client/graphics/layers/RadialMenuElements.ts (2)
490-492:⚠️ Potential issue | 🟠 MajorKeep the explicit
params === undefinedguard here.This branch reads
params.selectedon Line 491. IfsubMenu()is reached with missing params, it still crashes before returning the empty array.Possible fix
subMenu: (params: MenuElementParams): MenuElement[] => { - if (!params.selected) return []; + if (params === undefined || params.selected === null) return [];Based on learnings, in
src/client/graphics/layers/RadialMenuElements.ts,subMenufunctions should use explicit checks likeif (params === undefined || params.selected === null)becauseparamscan be undefined during spawn phase operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/RadialMenuElements.ts` around lines 490 - 492, The subMenu implementation reads params.selected and can crash when params is undefined during spawn phase; update the guard in the subMenu function (MenuElementParams) to explicitly check for a missing params object and null selected (e.g., if (params === undefined || params.selected === null) return []), ensuring you reference the subMenu function and MenuElementParams type when making the change.
439-460:⚠️ Potential issue | 🟠 MajorBlock the trade preset on self-targets.
This submenu also opens on your own territory. On Line 457 the action can still call
handleEmbargo()withmyPlayer, which is not a valid trade toggle. At minimum block self here; if the normal trade flow has stricter eligibility, reuse that same check too.Possible fix
if (slot.type === "trade") { return { id: "preset-trade-toggle", name: "trade", - disabled: (p: MenuElementParams) => p.selected === null, + disabled: (p: MenuElementParams) => + p.selected === null || p.selected.id() === p.myPlayer.id(), color: (p: MenuElementParams) => p.playerActions?.interaction?.canEmbargo ? COLORS.embargo : COLORS.trade, @@ }, fontSize: "10px", action: (p: MenuElementParams) => { + if (p.selected === null || p.selected.id() === p.myPlayer.id()) { + return; + } const canEmbargo = !!p.playerActions?.interaction?.canEmbargo; p.playerActionHandler.handleEmbargo( p.selected!, canEmbargo ? "start" : "stop",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/RadialMenuElements.ts` around lines 439 - 460, The trade submenu allows acting on your own territory so handleEmbargo can be called with the current player; update the trade menu's disabled predicate and action guard to block self-targets (and ideally reuse any existing trade-eligibility check). Specifically, in the object returned for slot.type === "trade" adjust the disabled function (MenuElementParams p) to also return true when p.selected refers to the current player (e.g., compare p.selected against the current player identity available on p), and in the action handler add an early-return guard that prevents calling p.playerActionHandler.handleEmbargo when p.selected is self (or when a shared trade eligibility check fails), preserving use of p.playerActions?.interaction?.canEmbargo and p.playerActionHandler.handleEmbargo.src/client/graphics/layers/QuickChatConfigModal.ts (2)
180-190:⚠️ Potential issue | 🟡 MinorGive the remove button an accessible name.
The icon-only
✕button has no label, so assistive tech cannot tell which preset it removes.Possible fix
${this.slots.length > 1 ? html`<button class="chat-option-button" style="padding:8px 10px;" + aria-label=${translateText("quick_chat.remove_preset", { + n: i + 1, + })} + title=${translateText("quick_chat.remove_preset", { + n: i + 1, + })} `@click`=${(e: Event) => { e.stopPropagation(); this.removeSlot(i); }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/QuickChatConfigModal.ts` around lines 180 - 190, The remove button in QuickChatConfigModal's render block is icon-only and lacks an accessible name; update the button (class "chat-option-button") to include an explicit accessible label (e.g., aria-label or visually-hidden text) that identifies which slot/preset it removes using the slot identifier from this.slots (or index i) so assistive tech can convey it, while keeping the existing click handler that calls removeSlot(i).
53-60:⚠️ Potential issue | 🟡 MinorClear the old reset timer before starting a new one.
This timeout survives
close()/open(). If the modal is reopened quickly, a stale callback can clear a fresh confirmation window early.Possible fix
`@customElement`("quick-chat-config-modal") export class QuickChatConfigModal extends LitElement { @@ `@state`() private selectedCategory: string | null = null; `@state`() private confirmReset = false; + private resetConfirmTimer: number | null = null; @@ open() { + if (this.resetConfirmTimer !== null) { + clearTimeout(this.resetConfirmTimer); + this.resetConfirmTimer = null; + } this.slots = QuickChatPresetService.getInstance().load(); @@ close() { + if (this.resetConfirmTimer !== null) { + clearTimeout(this.resetConfirmTimer); + this.resetConfirmTimer = null; + } this.editingIndex = null; @@ private reset() { + if (this.resetConfirmTimer !== null) { + clearTimeout(this.resetConfirmTimer); + this.resetConfirmTimer = null; + } if (!this.confirmReset) { this.confirmReset = true; this.requestUpdate(); - setTimeout(() => { + this.resetConfirmTimer = window.setTimeout(() => { this.confirmReset = false; + this.resetConfirmTimer = null; this.requestUpdate(); }, 3000); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/QuickChatConfigModal.ts` around lines 53 - 60, The reset() method currently starts a new setTimeout without clearing any previous timer, so a stale timeout can flip confirmReset=false after the modal is reopened; store the timeout id on the instance (e.g., this.resetTimer), call clearTimeout(this.resetTimer) before creating a new timeout in reset(), and also clear that timer in close()/open() (or wherever the modal is torn down) to ensure no stale callback flips confirmReset; update references to use this.resetTimer and ensure requestUpdate() is still called after changing confirmReset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/graphics/layers/QuickChatConfigModal.ts`:
- Around line 180-190: The remove button in QuickChatConfigModal's render block
is icon-only and lacks an accessible name; update the button (class
"chat-option-button") to include an explicit accessible label (e.g., aria-label
or visually-hidden text) that identifies which slot/preset it removes using the
slot identifier from this.slots (or index i) so assistive tech can convey it,
while keeping the existing click handler that calls removeSlot(i).
- Around line 53-60: The reset() method currently starts a new setTimeout
without clearing any previous timer, so a stale timeout can flip
confirmReset=false after the modal is reopened; store the timeout id on the
instance (e.g., this.resetTimer), call clearTimeout(this.resetTimer) before
creating a new timeout in reset(), and also clear that timer in close()/open()
(or wherever the modal is torn down) to ensure no stale callback flips
confirmReset; update references to use this.resetTimer and ensure
requestUpdate() is still called after changing confirmReset.
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 490-492: The subMenu implementation reads params.selected and can
crash when params is undefined during spawn phase; update the guard in the
subMenu function (MenuElementParams) to explicitly check for a missing params
object and null selected (e.g., if (params === undefined || params.selected ===
null) return []), ensuring you reference the subMenu function and
MenuElementParams type when making the change.
- Around line 439-460: The trade submenu allows acting on your own territory so
handleEmbargo can be called with the current player; update the trade menu's
disabled predicate and action guard to block self-targets (and ideally reuse any
existing trade-eligibility check). Specifically, in the object returned for
slot.type === "trade" adjust the disabled function (MenuElementParams p) to also
return true when p.selected refers to the current player (e.g., compare
p.selected against the current player identity available on p), and in the
action handler add an early-return guard that prevents calling
p.playerActionHandler.handleEmbargo when p.selected is self (or when a shared
trade eligibility check fails), preserving use of
p.playerActions?.interaction?.canEmbargo and
p.playerActionHandler.handleEmbargo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08e90b77-931d-4317-a070-9f3c4337c3e5
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/QuickChatConfigModal.tssrc/client/graphics/layers/RadialMenuElements.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
baculinivan-web
force-pushed
the
feature/quick-chat-radial-menu
branch
from
f169d07 to
d350eca
Compare
Contributor
✅ Actions performedComments resolved and changes approved. |
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/graphics/layers/RadialMenuElements.ts (1)
443-447:⚠️ Potential issue | 🟡 MinorCompare players by ID, not by object reference.
Using
p.selected === p.myPlayercompares object references. If these are different instances representing the same player, the self-trade guard fails silently. Compare by ID for reliability.Proposed fix
disabled: (p: MenuElementParams) => p.selected === null || - p.selected === p.myPlayer || + p.selected.id() === p.myPlayer.id() || (!p.playerActions?.interaction?.canEmbargo && !p.playerActions?.interaction?.canDonateGold),action: (p: MenuElementParams) => { if ( p.selected === null || - p.selected === p.myPlayer || + p.selected.id() === p.myPlayer.id() || (!p.playerActions?.interaction?.canEmbargo && !p.playerActions?.interaction?.canDonateGold) ) return;Also applies to: 460-466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/RadialMenuElements.ts` around lines 443 - 447, The current disabled predicate in RadialMenuElements (the function using MenuElementParams) compares players by object reference (p.selected === p.myPlayer), which can fail if different instances represent the same player; update both occurrences (the disabled check at the shown block and the similar block around lines 460-466) to compare player IDs instead (e.g., check p.selected?.id === p.myPlayer?.id), guard against null/undefined for p.selected and p.myPlayer, and keep the rest of the boolean logic unchanged so self-trade detection uses ID equality rather than object identity.
🧹 Nitpick comments (1)
src/client/graphics/layers/RadialMenuElements.ts (1)
537-545: Consider adding a typed interface for the modal element.Casting to
anyworks but loses type safety. A small interface would make intent clearer and catch typos.Optional improvement
+interface QuickChatConfigModal extends HTMLElement { + open(): void; +} action: (p: MenuElementParams) => { - const modal = document.querySelector("quick-chat-config-modal") as any; + const modal = document.querySelector<QuickChatConfigModal>("quick-chat-config-modal"); if (modal) { modal.open(); } else { console.error("[QuickChat] quick-chat-config-modal not found"); } p.closeMenu(); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/RadialMenuElements.ts` around lines 537 - 545, Replace the unsafe any cast for the modal with a small typed interface and use it when querying the element: declare an interface (e.g., QuickChatConfigModal) that includes the methods/properties you call (open(): void) and then change the cast in the action block to querySelector("quick-chat-config-modal") as QuickChatConfigModal | null, check for null, call modal.open(), and preserve p.closeMenu(); this keeps type safety for the modal usage in the action lambda that accepts MenuElementParams.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 443-447: The current disabled predicate in RadialMenuElements (the
function using MenuElementParams) compares players by object reference
(p.selected === p.myPlayer), which can fail if different instances represent the
same player; update both occurrences (the disabled check at the shown block and
the similar block around lines 460-466) to compare player IDs instead (e.g.,
check p.selected?.id === p.myPlayer?.id), guard against null/undefined for
p.selected and p.myPlayer, and keep the rest of the boolean logic unchanged so
self-trade detection uses ID equality rather than object identity.
---
Nitpick comments:
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 537-545: Replace the unsafe any cast for the modal with a small
typed interface and use it when querying the element: declare an interface
(e.g., QuickChatConfigModal) that includes the methods/properties you call
(open(): void) and then change the cast in the action block to
querySelector("quick-chat-config-modal") as QuickChatConfigModal | null, check
for null, call modal.open(), and preserve p.closeMenu(); this keeps type safety
for the modal usage in the action lambda that accepts MenuElementParams.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 379611b1-3386-45dd-a803-6527ea759667
📒 Files selected for processing (1)
src/client/graphics/layers/RadialMenuElements.ts
baculinivan-web
force-pushed
the
feature/quick-chat-radial-menu
branch
from
765c834 to
373735c
Compare
Contributor
Author
|
In dev Discord one person pointed out that such a change in the menu can create some dissatisfaction. He asked if we can implement keybindings for chat. E.g.: [C] + [1] + [2] send message from category 1 - help, message 1 - give me troops. I think that while the keybinding is pressed a little cheatsheet can be shown for easier access. |
Collaborator
|
We actually had this implemented about 6 months ago but decided not to move forward because it increases the number of clicks to view the player profile. We need to rethink how the UI is going to look. I'm thinking of using keybindings, like hover over player, press a key, and that opens the quick chat menu. |
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
4 participants
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.
Resolves #3518
Description:
Replaces the "Info" button in the right-click radial menu with a Quick Chat submenu. Players can send preset chat messages, open the emoji panel, or toggle trade — all from the radial menu without opening the full chat.
What's included:
requiresPlayer): cursor changes to a pointer with a badge, clicking an enemy tile sends the message targeting that playertranslateText()with newquick_chat.*i18n keys inen.jsonOther changes:
ClientGameRunnerandUnitLayerclick handlers during target selection to prevent accidental attacksisRightClickflag toContextMenuEventto distinguish right-click from left-click.kiro/to.gitignore(kiro - ai agent harness)tests/setup.tsfor jsdom environmentsPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
fghjk_60845
UX and UI videos:
CleanShot.2026-03-28.at.01.24.38.mp4
CleanShot.2026-03-28.at.01.23.15.mp4
CleanShot.2026-03-28.at.01.21.57.mp4
CleanShot.2026-03-28.at.01.27.06.mp4