Skip to content

Rework the chat window - #7098

Merged
Garanas merged 136 commits into
FAForever:developfrom
Garanas:refactor/chat
Jun 20, 2026
Merged

Garanas merged 136 commits into
FAForever:developfrom
Garanas:refactor/chat

Conversation

@Garanas

@Garanas Garanas commented Apr 26, 2026

Copy link
Copy Markdown
Member

Description of the proposed changes

See #7098 (comment) for some visuals

I was playing a game recently and someone shared me some units. I could not find them. Then I thought: let's add a chat message with some camera information in it in order to find them easier next time. That turned out to be quite hard in the original implementation, I could not figure it out.

On work I've been using Claude quite a bit. It is quite powerful. But what better to test its capabilities then on a code base, with a framework that the LLM did not train on indefinitely? One thing led to another, and here we have an entirely new chat window.

This chat window is implemented using the MVC-principle, to keep the model, the controller and the interface cleanly separated. I've learned from my last attempts with the autolobby, this time no property drilling but proper use of LazyVars.

In general all functionality of the existing chat window also exists in the new chat window. There are a few subtle differences, but none that are breaking in my point of view:

  • The chat (and the feed) starts bottom-up, instead of top-down.
  • The chat feed does not persist when opening/closing the chat window. I am not talking about the chat history here, but about the feed that shows when you receive a chat message but you do not have the chat window open.
  • The chat is a single window/rect now. The legacy SetLayout rotated through bottom / left / right HUD layouts; the new chat lets the user drag and resize freely instead.
  • Idle fade is now activity-based rather than a window-level wall-clock timer. Any interaction (keystrokes, scrolling, recipient hovers, incoming messages) re-stamps a LastActivity LazyVar; the window's OnFrame checks now − LastActivity against fade_time. Net effect: a burst of incoming chat keeps the window open instead of auto-closing mid-conversation. Unchecking the pin also re-stamps activity, so you get a full fade_time window instead of being closed instantly.

In comparison, we receive a lot of new features and capabilities:

  • The chat window fully supports UI scaling. Saved window positions are stored inverse-scaled and re-applied on next launch, so a user who changes ui_scale doesn't end up with a half-off-screen chat window.
  • The chat window will now always snap to frame(0) when opened, so that you can always view the chat even after changing resolution.
  • The chat window is now almost a command line interface where you can issue commands using /, these are simple to extend for UI mods. Built-ins include /all, /allies, /whisper (/w / /pm), /help, /gift-resources, /gift-units, /recall, /mute, /unmute, /clear, /restart, /save, /load, /pause, /resume, /speed, /end-mission, /to-engineers, /to-tick, /taunt, plus a few /debug-… helpers.
  • The chat window now supports various forms of auto complete. When you use /, you get a few suggestions of available commands in a popup above the edit box. When you use @ (or just press Tab on a partial nickname) you can auto-complete player names. Tab cycles through candidates; commands like /whisper @Jip accept the @-prefixed form too.
  • The chat window now mutes players on a technically feasible manner. The mute filter is no longer persisted, but we can adjust this accordingly. It is also reachable via the /mute @Nick and /unmute @Nick commands.
  • The simulation can send chat messages much more conveniently than before. Sim-side senders can also attach a lightweight Location hint (a world point or rectangle) instead of a full camera snapshot — useful for AI brains that don't have a saved view to ship.
  • The chat messages can now be localized, useful for system, event or AI messages.
  • Every top-level view (chat window, options dialog, edit area) is independently callable from a hotkey or the console with UI_Lua import(…).Toggle(). Useful for debugging in isolation, and the keybinding UI now exposes them under the chat category.
  • Hot reload support: saving any chat module rebuilds the in-game UI in place, with the model preserving history across the reload. Made the development cycle a lot faster.

We fix some bugs:

  • The UI scaling now works properly on all elements.
  • The scrollbar fades out properly when the window fades.
  • A new sim callback is introduced for chat messages instead of (ab)using the GiftResourcesToPlayer callback. The old sim callback is still emitted, but now once per message instead of X − 1 per player per message.
  • Incoming messages are shape-validated; modded / hostile / malformed payloads are dropped silently instead of being coerced. Legacy trusted whatever shape arrived on the wire.

And of course, what we started with:

  • When you gift units you receive a whisper with a camera location of where those units are.
  • When you gift resources you receive a whisper of the amount received.

Checklist

  • Check compatibility with replay parsing.

    This should be untouched, but then again - one can't know for sure. Better be safe than sorry here.

  • Check whether the way AIs use the chat still works.

I did some basic testing using the new methods, called from the ai brain this works:

ForkThread (
    function()
        WaitTicks(10)
        self:SendChatToAll("Easy AI engaged! This is a work in progress, expect weird behaviour and crashes. Report any issues on the forums with a detailed description of what happened and a save file if possible.")
        self:SendChatToPlayer(1, "I see yooouuu")
    end
)

Testing done on the proposed changes

Tested locally with multiple players using the launch script inside the scripts folder. Messages can be send just fine.

Debug key actions

All actions are filed under the chat category in the keybindings dialog (so they group with regular chat actions) but ship without default keys — bind them via the keybindings UI or invoke from the console with UI_Lua import("/lua/ui/game/chat/ChatDebug.lua").Foo().

Action Description
debug_chat_window Toggle the chat window.
debug_chat_config Toggle the chat options dialog.
debug_chat_append_system_message Append a local-only System: line — exercises AppendLocalSystemMessage and the system colour.
debug_chat_append_short_message Append one synthetic chat entry stamped with the local focus army's metadata — exercises the basic model→view path and snap-to-bottom.
debug_chat_append_long_message Append a multi-paragraph entry — exercises WrapEntry and the continuation-row layout at every supported font size.
debug_chat_append_burst Append ten synthetic entries in one batch — exercises pool sizing past the line cap, virtual-size accounting, and snap-to-bottom on rapid arrivals.
debug_chat_append_camera_message Append an entry with a Location hint at the current camera focus — exercises the cam-icon toggle on the row and Camera:MoveTo on click (press, pan away, click the icon — camera bounces back).
debug_chat_set_recipient_all Force the send target to all — exercises the recipient-label LazyVar binding.
debug_chat_set_recipient_allies Force the send target to allies.
debug_chat_clear_history Wipe the history log — exercises the empty-pool branch of CalcVisible and model-side dirty propagation.

Additional context

@BlackYps I don't think it makes sense to review this pull request line by line. Instead, it's a question of whether we want this as a whole or not. Yes, it will break a few things. Yes, it won't be perfect immediately. But, in my humble opinion, it is an improvement that makes the chat window maintainable again for the foreseeable future.

To be honest: I would not mind if this gets closed. The goal was to experiment with Claude and spec driven development on a code base that it did not quite train as much on as regular programming languages and/or frameworks. But it would be a loss in my opinion 😃 !

See #7098 (comment) for some visuals

Checklist

Summary by CodeRabbit

  • New Features
    • Complete chat system overhaul with redesigned UI, command palette, and autocomplete.
    • New slash commands: /all, /allies, /whisper, /mute, /unmute, /gift-units, /gift-resources, /save, /load, /pause, /resume, /recall, /help, and more.
    • Chat configuration dialog for colors, font size, fade time, and window opacity.
    • Player muting feature.
    • Localized chat notifications for resource and unit transfers (German, Russian, English).

Garanas added 30 commits April 18, 2026 21:29

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (4)
lua/ui/game/chat/ChatLineInterface.lua (1)

94-95: Reuse DefaultBodyColor instead of duplicating the literal.

'ffc2f6ff' here is the same magic value as the DefaultBodyColor constant declared at Line 17. The init color is overwritten by SetHeader/SetContinuation/Clear on first use, but keeping a duplicate literal is a quiet drift hazard.

♻️ Proposed fix
         self.Text = UIUtil.CreateText(self, '', 14, 'Arial')
-        self.Text:SetColor('ffc2f6ff')
+        self.Text:SetColor(DefaultBodyColor)
         self.Text:SetDropShadow(true)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lua/ui/game/chat/ChatLineInterface.lua` around lines 94 - 95, Replace the
duplicated literal color with the existing constant: in the ChatLineInterface
initializer where self.Text is created (UIUtil.CreateText(...)) and its color
set via self.Text:SetColor('ffc2f6ff'), use the predefined DefaultBodyColor
constant instead; update the call to SetColor to reference DefaultBodyColor so
the class (ChatLineInterface) consistently reuses the same color value that
SetHeader/SetContinuation/Clear expect.
lua/ui/game/chat/ChatUtils.lua (1)

53-65: Pre-scale the literal + 4 gap to keep wrap consistent across UI scales.

measureLine.Right(), Name.Left(), and Name:GetStringAdvance(name) are already in scaled pixels, so adding an unscaled 4 shrinks the reserved gap proportionally as ui_scale grows. Capture a scaled constant once and reuse it.

♻️ Proposed fix
 function WrapEntry(entry, measureLine)
     if not measureLine then
         entry.WrappedText = { entry.Text or '' }
         return
     end

     local name = entry.Name or ''
+    local gap = LayoutHelpers.ScaleNumber(4)
     local lines = MauiWrapText(entry.Text or '',
         function(lineIndex)
             if lineIndex == 1 then
                 return measureLine.Right()
-                    - (measureLine.Name.Left() + measureLine.Name:GetStringAdvance(name) + 4)
+                    - (measureLine.Name.Left() + measureLine.Name:GetStringAdvance(name) + gap)
             else
                 return measureLine.Right()
-                    - (measureLine.Name.Left() + 4)
+                    - (measureLine.Name.Left() + gap)
             end
         end,

This requires importing LayoutHelpers at the top:

 local MauiWrapText = import("/lua/maui/text.lua").WrapText
 local ChatPayload = import("/lua/shared/ChatPayload.lua")
+local LayoutHelpers = import("/lua/maui/layouthelpers.lua")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lua/ui/game/chat/ChatUtils.lua` around lines 53 - 65, The literal "+ 4" must
be scaled with the UI scale and reused: import LayoutHelpers (or ensure it's
available), compute a local scaled gap (e.g., local gap =
LayoutHelpers.ScaleNumber(4)) and replace both occurrences of " + 4" inside the
MauiWrapText width function with that scaled gap so measureLine.Right(),
measureLine.Name.Left(), and measureLine.Name:GetStringAdvance(name) remain in
scaled pixels and the reserved gap stays consistent across ui_scale; keep the
new local variable near where measureLine is used so MauiWrapText's width
callbacks reference gap instead of the unscaled 4.
lua/ui/game/chat/ChatLinesInterface.lua (1)

238-241: Redundant RefreshVirtualSize call in ApplyOptions.

self:RewrapAll() at line 239 already invokes self:RefreshVirtualSize(history) (line 264), so the explicit call at line 241 walks the entire history a second time on every committed-options change. Drop the duplicate to keep the option-change path single-pass.

♻️ Proposed cleanup
         self:RebuildPool()
         self:RewrapAll()
 
-        self:RefreshVirtualSize()
         self:RecomputeScrollTopForPoolChange(oldPoolSize)
         self:CalcVisible()

As per coding guidelines: "WrappedText cache on UIChatEntry should be updated by ChatLinesInterface when entry text or row width changes" — RewrapAll is already the canonical update path and refreshes virtual size as part of that cycle.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lua/ui/game/chat/ChatLinesInterface.lua` around lines 238 - 241, Remove the
redundant call to RefreshVirtualSize from ApplyOptions: after calling
self:RebuildPool() and self:RewrapAll() the virtual size is already recomputed
by RewrapAll (which calls self:RefreshVirtualSize(history)), so delete the
explicit self:RefreshVirtualSize() invocation in ApplyOptions to avoid a second
full-history pass; keep RebuildPool and RewrapAll as-is so wrapped-text caching
and virtual size updates remain driven by RewrapAll.
lua/ui/game/chat/ChatController.lua (1)

283-285: Wrap LOCF with pcall to handle mismatched Args gracefully.

ChatPayload.IsValidPayload validates that Args is a table (if present) but does not verify its contents match the format specifiers in msg.text. A malformed payload like { text='%d', Args={'oops'} } will cause string.format to fail. Use pcall to catch the error and fall back to the raw template:

Hardening
    if msg.Args then
-       msg.text = LOCF(msg.text, unpack(msg.Args))
+       local ok, formatted = pcall(LOCF, msg.text, unpack(msg.Args))
+       if ok then
+           msg.text = formatted
+       end
    end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lua/ui/game/chat/ChatController.lua` around lines 283 - 285, Wrap the
LOCF(msg.text, unpack(msg.Args)) call in a pcall to prevent a malformed Args
table from causing an error; in ChatController.lua where you currently have the
LOCF invocation (handling msg.Args), call pcall(LOCF, msg.text,
unpack(msg.Args)) and only replace msg.text when pcall returns true, otherwise
leave msg.text as the original template and optionally log or ignore the format
failure; this change hardens handling around msg.Args and works with existing
ChatPayload.IsValidPayload validations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lua/ui/game/chat/ChatController.lua`:
- Around line 268-328: OnReceive currently misses id-based deduping promised by
the docstring; add an early check to skip messages whose msg.Id is already in
the chat history and ensure messages processed here seed the history after
rendering. Concretely: in OnReceive, before any rendering/LOCF/Notify logic
consult ChatModel.history (or the module's history table used by
OnSyncChatMessages) and if msg.Id and history[msg.Id] then return; after
AppendChatLine succeeds, if msg.Id then set history[msg.Id] = true so future
OnReceive/OnSyncChatMessages calls will be deduped. Reference: OnReceive,
msg.Id, AppendChatLine, OnSyncChatMessages, ChatModel.history (history table).

In `@lua/ui/game/chat/ChatListInterface.lua`:
- Around line 62-72: The global click handler onOutsideClick currently calls
self:Destroy() unconditionally via UIMain.AddOnMouseClickedFunc, which preempts
control-level handlers; modify onOutsideClick to accept the event parameter and
only call self:Destroy() when the click is outside this popup's bounds by
checking event.x/event.y against this control's Left()/Right()/Top()/Bottom();
apply the same bounds-guarded pattern to the other onOutsideClick instance
around the 126–136 range and ensure UIMain.RemoveOnMouseClickedFunc is still
called in self.OnDestroy and _OnClosed behavior remains unchanged.
- Around line 162-174: The popup layout is double-scaling dimensions because
__post_init computes maxWidth/totalHeight from already-scaled entry.Text: do not
pass those raw numbers into Layouter:Width()/Height() which auto-scale; instead
call Layouter:SetFunction("Width", function() return maxWidth + 40 end) and
Layouter:SetFunction("Height", function() return totalHeight end) (or otherwise
use :SetFunction for the same keys) so the values bypass Layouter's internal
ScaleNumber; update the __post_init block around the
Layouter(self):Width(...):Height(...):End() call to use :SetFunction for "Width"
and "Height" referencing maxWidth and totalHeight computed from
entry.Text.Width()/Height().

In `@lua/ui/game/chat/config/ChatConfigController.lua`:
- Around line 69-80: SetMutedLive currently mutates only Committed (via Model()
and model.Committed:Set) causing Pending to remain stale and later Apply to
overwrite the live change; update SetMutedLive to mirror the same mutated
options into Pending as well (i.e., after computing options.muted and calling
model.Committed:Set(options), also update model.Pending:Set(...) or the
equivalent Pending API so the dialog’s Pending state and the PendingObserver in
ChatConfigInterface.lua remain in sync with the live mute change).

In `@lua/ui/game/chat/config/ChatConfigInterface.lua`:
- Around line 418-420: The OnClose handler currently calls Close() only, leaving
Pending edits intact; change the OnClose closure to call Cancel() followed by
Close() (i.e., mirror the explicit Cancel button behavior) so X discards drafts.
Additionally, modify Open (the function that currently calls Show() on the
existing instance) to re-sync Pending from Committed before calling Show() —
ensure Open resets the dialog's Pending state to the current Committed values
(e.g., copy/mirror Committed -> Pending or call the existing reset logic) so
reopening always reflects up-to-date committed settings and avoids clobbering
intervening SetMutedLive changes.
- Around line 39-51: The UI strings in ColorDefs and CheckboxDefs (e.g., Text
values "All","Allies","Private","Links","Notify" and "Default recipient:
allies","Show feed background","Show camera links") are hardcoded English; wrap
every user-visible Text/Tooltip and button/label strings referenced by
ChatConfigInterface (including slider preview labels generated with
string.format) with the localization token pattern <LOC ...>...LOC so they go
through the localization pipeline, and replace any string.format("Font Size:
%d", value) style formatting with a localized format token (e.g., a <LOC> token
that includes a placeholder) so the numeric value is interpolated via the
localization-aware formatting; update the entries in ColorDefs and CheckboxDefs
and the slider preview/label code paths accordingly (use the same unique
keys/identifiers currently used in the diff so callers like the slider preview
and any UI builders pick up the localized strings).

---

Nitpick comments:
In `@lua/ui/game/chat/ChatController.lua`:
- Around line 283-285: Wrap the LOCF(msg.text, unpack(msg.Args)) call in a pcall
to prevent a malformed Args table from causing an error; in ChatController.lua
where you currently have the LOCF invocation (handling msg.Args), call
pcall(LOCF, msg.text, unpack(msg.Args)) and only replace msg.text when pcall
returns true, otherwise leave msg.text as the original template and optionally
log or ignore the format failure; this change hardens handling around msg.Args
and works with existing ChatPayload.IsValidPayload validations.

In `@lua/ui/game/chat/ChatLineInterface.lua`:
- Around line 94-95: Replace the duplicated literal color with the existing
constant: in the ChatLineInterface initializer where self.Text is created
(UIUtil.CreateText(...)) and its color set via self.Text:SetColor('ffc2f6ff'),
use the predefined DefaultBodyColor constant instead; update the call to
SetColor to reference DefaultBodyColor so the class (ChatLineInterface)
consistently reuses the same color value that SetHeader/SetContinuation/Clear
expect.

In `@lua/ui/game/chat/ChatLinesInterface.lua`:
- Around line 238-241: Remove the redundant call to RefreshVirtualSize from
ApplyOptions: after calling self:RebuildPool() and self:RewrapAll() the virtual
size is already recomputed by RewrapAll (which calls
self:RefreshVirtualSize(history)), so delete the explicit
self:RefreshVirtualSize() invocation in ApplyOptions to avoid a second
full-history pass; keep RebuildPool and RewrapAll as-is so wrapped-text caching
and virtual size updates remain driven by RewrapAll.

In `@lua/ui/game/chat/ChatUtils.lua`:
- Around line 53-65: The literal "+ 4" must be scaled with the UI scale and
reused: import LayoutHelpers (or ensure it's available), compute a local scaled
gap (e.g., local gap = LayoutHelpers.ScaleNumber(4)) and replace both
occurrences of " + 4" inside the MauiWrapText width function with that scaled
gap so measureLine.Right(), measureLine.Name.Left(), and
measureLine.Name:GetStringAdvance(name) remain in scaled pixels and the reserved
gap stays consistent across ui_scale; keep the new local variable near where
measureLine is used so MauiWrapText's width callbacks reference gap instead of
the unscaled 4.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: fcffe53d-b331-4bf0-90bf-4d70221471c6

📥 Commits

Reviewing files that changed from the base of the PR and between d04350f and 9002d91.

📒 Files selected for processing (17)
  • lua/ui/controls/floattext.lua
  • lua/ui/game/chat/ChatCommandHintInterface.lua
  • lua/ui/game/chat/ChatCompletion.lua
  • lua/ui/game/chat/ChatController.lua
  • lua/ui/game/chat/ChatDebug.lua
  • lua/ui/game/chat/ChatEditInterface.lua
  • lua/ui/game/chat/ChatFactionBadge.lua
  • lua/ui/game/chat/ChatFeedInterface.lua
  • lua/ui/game/chat/ChatInterface.lua
  • lua/ui/game/chat/ChatLineInterface.lua
  • lua/ui/game/chat/ChatLinesInterface.lua
  • lua/ui/game/chat/ChatListInterface.lua
  • lua/ui/game/chat/ChatModel.lua
  • lua/ui/game/chat/ChatUtils.lua
  • lua/ui/game/chat/config/ChatConfigController.lua
  • lua/ui/game/chat/config/ChatConfigInterface.lua
  • lua/ui/game/chat/config/ChatConfigModel.lua
✅ Files skipped from review due to trivial changes (3)
  • lua/ui/game/chat/ChatFactionBadge.lua
  • lua/ui/game/chat/ChatDebug.lua
  • lua/ui/game/chat/ChatEditInterface.lua
🚧 Files skipped from review as they are similar to previous changes (2)
  • lua/ui/game/chat/ChatCompletion.lua
  • lua/ui/game/chat/config/ChatConfigModel.lua

Comment thread lua/ui/game/chat/ChatController.lua
Comment thread lua/ui/game/chat/ChatListInterface.lua
Comment thread lua/ui/game/chat/ChatListInterface.lua
Comment thread lua/ui/game/chat/config/ChatConfigController.lua
Comment on lines +39 to +51
local ColorDefs = {
{ Key = ChatConfigModel.KeyAllColor, Text = "All", Tooltip = 'chat_color' },
{ Key = ChatConfigModel.KeyAlliesColor, Text = "Allies", Tooltip = 'chat_color' },
{ Key = ChatConfigModel.KeyPrivColor, Text = "Private", Tooltip = 'chat_color' },
{ Key = ChatConfigModel.KeyLinkColor, Text = "Links", Tooltip = 'chat_color' },
{ Key = ChatConfigModel.KeyNotifyColor, Text = "Notify", Tooltip = 'chat_color' },
}

local CheckboxDefs = {
{ Key = ChatConfigModel.KeySendType, Text = "Default recipient: allies", Tooltip = 'chat_send_type' },
{ Key = ChatConfigModel.KeyFeedBackground, Text = "Show feed background", Tooltip = 'chat_feed_background' },
{ Key = ChatConfigModel.KeyLinks, Text = "Show camera links", Tooltip = 'chat_filter' },
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Hardcoded English UI strings — inconsistent with the PR's localization story.

The PR objectives call out localized system/AI/event messages, but every label/title/button text in this dialog is a raw English string ("Chat Configuration", "Message Colors", "All", "Allies", "Font Size: 14", "Apply", "OK", "Cancel", etc.). Wrap user-visible strings with <LOC ...>/LOC so they go through the localization pipeline like the rest of the UI.

Also worth noting: the slider preview labels use string.format("Font Size: %d", value) etc., which won't survive translation either — the formatted prefix needs to come from a <LOC> token with the value interpolated.

Also applies to: 97-99, 106-106, 133-133, 148-148, 163-163, 179-179, 195-195, 216-216, 222-222, 225-225, 231-231

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lua/ui/game/chat/config/ChatConfigInterface.lua` around lines 39 - 51, The UI
strings in ColorDefs and CheckboxDefs (e.g., Text values
"All","Allies","Private","Links","Notify" and "Default recipient: allies","Show
feed background","Show camera links") are hardcoded English; wrap every
user-visible Text/Tooltip and button/label strings referenced by
ChatConfigInterface (including slider preview labels generated with
string.format) with the localization token pattern <LOC ...>...LOC so they go
through the localization pipeline, and replace any string.format("Font Size:
%d", value) style formatting with a localized format token (e.g., a <LOC> token
that includes a placeholder) so the numeric value is interpolated via the
localization-aware formatting; update the entries in ColorDefs and CheckboxDefs
and the slider preview/label code paths accordingly (use the same unique
keys/identifiers currently used in the diff so callers like the slider preview
and any UI builders pick up the localized strings).

Comment thread lua/ui/game/chat/config/ChatConfigInterface.lua
@Garanas Garanas added ui: chat window related to chatting with players area: ui Anything to do with the User Interface of the Game labels Apr 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
lua/aibrains/components/ChatBrainComponent.lua (1)

101-115: Consider validating the payload before relaying.

SendChatTo only checks that text is a non-empty string before calling ChatUtils.RelayChatMessage, which (unlike the UI-originated ChatUtils.SendChatMessage) does not run ChatPayload.IsValidPayload. As a result, an AI/campaign caller can leak a payload into Sync.ChatMessages that the shared validator was designed to reject:

  • text longer than ChatPayload.MaxMessageLength (200 UTF‑8 chars) — silently passes.
  • to other than 'all' | 'allies' | integer (e.g., 'notify', a typo, or a stale recipient table) — silently dropped by IsLocalRecipient, with no feedback to the caller.
  • args passed as a non-table, or location as a non-table — these would normally be rejected by IsValidPayload, but here they reach the UI's LOCF/camera-link handlers and can crash on click.

Since RelayChatMessage is documented as the shared sim-side entry point with policy enforced "in exactly one place", running ChatPayload.IsValidPayload(msg) here (or moving the check into RelayChatMessage) would close the gap and give AI authors a deterministic failure mode rather than a silently dropped or malformed line.

♻️ Suggested change
+local ChatPayload = import("/lua/shared/ChatPayload.lua")
+
 ...
     SendChatTo = function(self, to, text, args, location)
         if type(text) ~= 'string' or text == '' then return end

         local msg = {
             Chat     = true,
             to       = to,
             text     = text,
             Args     = args,
             From     = self:GetArmyIndex(),
             location = location,
         }
         msg.Id = tostring(msg)

+        if not ChatPayload.IsValidPayload(msg) then
+            WARN(string.format("AIChatBrainComponent: dropped malformed chat from army %s (to=%s, len=%d)",
+                tostring(msg.From), tostring(to), STR_Utf8Len(text)))
+            return
+        end
+
         ChatUtils.RelayChatMessage(msg)
     end,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lua/aibrains/components/ChatBrainComponent.lua` around lines 101 - 115,
SendChatTo currently only checks that text is a non-empty string and then calls
ChatUtils.RelayChatMessage, which lets invalid chat payloads reach
Sync.ChatMessages; call ChatPayload.IsValidPayload(msg) inside SendChatTo (or
move the check into ChatUtils.RelayChatMessage) before relaying, and if it
returns false reject the message (log a descriptive error including
ChatPayload.MaxMessageLength and the offending fields like to/Args/location) so
malformed payloads (bad recipient values that IsLocalRecipient would drop,
non-table Args/location, or overly long text) are caught and not sent to
RelayChatMessage/LOCF.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@lua/aibrains/components/ChatBrainComponent.lua`:
- Around line 101-115: SendChatTo currently only checks that text is a non-empty
string and then calls ChatUtils.RelayChatMessage, which lets invalid chat
payloads reach Sync.ChatMessages; call ChatPayload.IsValidPayload(msg) inside
SendChatTo (or move the check into ChatUtils.RelayChatMessage) before relaying,
and if it returns false reject the message (log a descriptive error including
ChatPayload.MaxMessageLength and the offending fields like to/Args/location) so
malformed payloads (bad recipient values that IsLocalRecipient would drop,
non-table Args/location, or overly long text) are caught and not sent to
RelayChatMessage/LOCF.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 632b0887-8395-45c0-b167-ebbd8a39aaba

📥 Commits

Reviewing files that changed from the base of the PR and between 9002d91 and 60bd41d.

📒 Files selected for processing (3)
  • lua/ChatUtils.lua
  • lua/aibrain.lua
  • lua/aibrains/components/ChatBrainComponent.lua
🚧 Files skipped from review as they are similar to previous changes (1)
  • lua/aibrain.lua

@Garanas

Garanas commented Apr 28, 2026

Copy link
Copy Markdown
Member Author

@BlackYps I mentioned in the pull request description that this is primarily an experiment with Claude, but that the output in my opinion is a decent improvement on the status quo.

The experiment is done on my end. I won't process further feedback until the game team is sure whether they want this alternative implementation of the chat window. Beyond perhaps excessive comments, I think in general this setup is much more maintainable then the current spaghetti code. But the choice is all yours.

Please let me know what the game team wants in the upcoming weeks. If the answer is no, then feel free to close it. I won't process any additional feedback until the game team decided to want to proceed.

@Garanas
Garanas merged commit aa746be into FAForever:develop Jun 20, 2026
3 of 5 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Jun 26, 2026
3 tasks
@coderabbitai coderabbitai Bot mentioned this pull request Aug 15, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ui Anything to do with the User Interface of the Game ui: chat window related to chatting with players

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants