refactor: extract lobby submodules for better maintainability"#7140
refactor: extract lobby submodules for better maintainability"#7140TimMasalme wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughEleven new Lua modules are added under ChangesLobby modularization
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Suggested labels
Suggested reviewers
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 11
🧹 Nitpick comments (7)
lua/ui/lobby/lobby_modules/maputils.lua (1)
160-174: ⚡ Quick winUnused variable
requestedFaction.
requestedFactionis assigned at line 162 but never used. Either remove the assignment or pass it toConvertObserverToPlayerif faction information is needed.♻️ Proposed fix to remove unused variable
elseif IsObserver(localPlayerID) then if lobbyComm:IsHost() then - local requestedFaction = GetSanitisedLastFaction() HostUtils.ConvertObserverToPlayer(FindObserverSlotForID(localPlayerID), slot) else🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/ui/lobby/lobby_modules/maputils.lua` around lines 160 - 174, The variable requestedFaction is assigned the result of GetSanitisedLastFaction() but is never used in the subsequent code. Remove the assignment of requestedFaction on the line where it gets assigned from GetSanitisedLastFaction(), as it serves no purpose in the current logic and only clutters the code. If this faction information is actually needed by HostUtils.ConvertObserverToPlayer, pass requestedFaction as a parameter to that function call instead.lua/ui/lobby/lobby_modules/optionsdialog.lua (1)
313-334: 💤 Low valueUnnecessary
if true thenwrapper.This block always executes. If it was for feature-flagging during development, consider removing the wrapper now.
♻️ Suggested removal
- if true then - --snowflakes count - local currentSnowFlakesCount = Prefs.GetFromCurrentProfile('SnowFlakesCount') or 100 - ... - end + --snowflakes count + local currentSnowFlakesCount = Prefs.GetFromCurrentProfile('SnowFlakesCount') or 100 + ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/ui/lobby/lobby_modules/optionsdialog.lua` around lines 313 - 334, Remove the unnecessary `if true then` wrapper and its corresponding `end` statement from the snowflakes option block. The condition always evaluates to true and serves no purpose. Keep all the content inside intact, including the `currentSnowFlakesCount` variable initialization, the `slider_SnowFlakes_Count_TEXT` text creation, the `slider_SnowFlakes_Count` slider initialization, and the `OnValueChanged` callback function. Simply unindent the code and remove both the opening and closing conditional statements.lua/ui/lobby/lobby_modules/hostutils.lua (1)
497-510: 💤 Low valueConsider using
LobbyComm.maxPlayerSlotsinstead of magic number 16.The hardcoded
MAXSLOT = 16duplicates the constant already available via the injectedLobbyComm.maxPlayerSlots. Using the injected constant ensures consistency if the value ever changes.♻️ Suggested change
function SendArmySettingsToServer() local armyIdx = 1 - local MAXSLOT = 16 - for slotNum = 1, MAXSLOT do + for slotNum = 1, LobbyComm.maxPlayerSlots do local playerInfo = gameInfo.PlayerOptions[slotNum]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/ui/lobby/lobby_modules/hostutils.lua` around lines 497 - 510, The SendArmySettingsToServer function uses a hardcoded magic number MAXSLOT with value 16 instead of leveraging the injected LobbyComm.maxPlayerSlots constant. Replace the local MAXSLOT variable declaration with a direct reference to LobbyComm.maxPlayerSlots in the for loop condition to ensure consistency and eliminate the duplicate constant definition.lua/ui/lobby/lobby_modules/createui.lua (1)
1789-1791: 💤 Low valueDead code: snowflake feature is disabled.
The
if false thenblock will never execute. If this is intentional (seasonal/debug feature), consider adding a comment explaining when it should be enabled, or remove it entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/ui/lobby/lobby_modules/createui.lua` around lines 1789 - 1791, The if false then block containing the CreateSnowFlakes(GUI) function call is dead code that will never execute. Either remove this entire block entirely if the snowflake feature is no longer needed, or if this is an intentional seasonal or debug feature that should be enabled at certain times, replace the false condition with a meaningful comment explaining when and how this feature should be activated (such as a configuration flag, seasonal check, or debug mode variable).lua/ui/lobby/lobby_modules/cpubenchmark.lua (2)
93-101: ⚡ Quick winAdd nil guard for slot structure access.
The
CPUTextclosure (line 95) accessesGUI.slots[slotNumber].CPUSpeedBar.CPUActualValuewithout checking ifGUI.slots[slotNumber]exists. While the tooltip is initially attached during slot creation (percreateui.lua:305-324), the closure may be invoked later if the slot structure changes. Consider adding a nil guard.🛡️ Suggested nil check
local CPUText = function() local CPUInfo - if GUI.slots[slotNumber].CPUSpeedBar.CPUActualValue then + if GUI.slots[slotNumber] and GUI.slots[slotNumber].CPUSpeedBar and GUI.slots[slotNumber].CPUSpeedBar.CPUActualValue then CPUInfo = GUI.slots[slotNumber].CPUSpeedBar.CPUActualValue else CPUInfo = LOC('<LOC lobui_0458>UnKnown')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/ui/lobby/lobby_modules/cpubenchmark.lua` around lines 93 - 101, The CPUText closure accesses GUI.slots[slotNumber].CPUSpeedBar.CPUActualValue without verifying that GUI.slots[slotNumber] exists, which could cause errors if the slot structure is modified after initial creation. Add a nil guard to check if GUI.slots[slotNumber] exists before attempting to access the nested CPUSpeedBar and CPUActualValue properties; if the slot is nil, return the fallback LOC('<LOC lobui_0458>UnKnown') message to ensure the function remains safe.
149-152: ⚡ Quick winAdd nil guards for GUI.rerunBenchmark structure.
Lines 149 and 151 access
GUI.rerunBenchmarkand.labelwithout checking existence. While these controls should be initialized during UI setup, adding guards makes the module more resilient to partial initialization or UI lifecycle changes.🛡️ Suggested nil checks
function StressCPU(waitTime) - GUI.rerunBenchmark:Disable() + if not GUI.rerunBenchmark then + WARN('StressCPU: GUI.rerunBenchmark not initialized') + return nil + end + GUI.rerunBenchmark:Disable() for i = waitTime, 1, -1 do - GUI.rerunBenchmark.label:SetText(i..'s') + if GUI.rerunBenchmark.label then + GUI.rerunBenchmark.label:SetText(i..'s') + end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/ui/lobby/lobby_modules/cpubenchmark.lua` around lines 149 - 152, Add nil guards before accessing GUI.rerunBenchmark and its label property. Before the Disable() call on GUI.rerunBenchmark at line 149 and before accessing GUI.rerunBenchmark.label for the SetText() call at line 151, add checks to verify that GUI.rerunBenchmark exists and that GUI.rerunBenchmark.label exists. This makes the code more resilient to cases where these UI elements may not be fully initialized or available during the UI lifecycle.lua/ui/lobby/lobby_modules/chathandler.lua (1)
58-60: ⚡ Quick winAdd nil guards for the nested property access chain.
The expression
GetLocalPlayerData():AsTable().Factionchains multiple calls without nil checks. If any intermediate value (GetLocalPlayerData(),:AsTable(), or.Faction) is nil, this will crash. Consider guarding each step or using a safer accessor pattern.🛡️ Suggested defensive check
if not chatPlayerColor then nameFont = UIUtil.bodyFont if Prefs.GetOption('faction_font_color') then - nameColor = import("/lua/skins/skins.lua").skins[ FACTION_NAMES[GetLocalPlayerData():AsTable().Faction] ].fontColor + local localData = GetLocalPlayerData() + if localData then + local dataTable = localData:AsTable() + if dataTable and dataTable.Faction and FACTION_NAMES[dataTable.Faction] then + nameColor = import("/lua/skins/skins.lua").skins[ FACTION_NAMES[dataTable.Faction] ].fontColor + end + end textColor = nameColor else🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/ui/lobby/lobby_modules/chathandler.lua` around lines 58 - 60, The nested property access chain in the line assigning nameColor from skins lacks nil guards and will crash if GetLocalPlayerData() returns nil, if AsTable() returns nil, or if the Faction property is nil. Add defensive nil checks for each step of the chain: first verify GetLocalPlayerData() is not nil, then verify the result of AsTable() is not nil, and finally verify that the Faction property exists and is not nil before attempting to access the skins array and retrieve the fontColor property. Only assign nameColor and textColor if all intermediate values are valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/ui/lobby/lobby_modules/aiutils.lua`:
- Around line 136-179: The generic for loop iterating over the aitypes table in
the GetAIPlayerData function is missing the required iterator function. Change
the loop statement from `for k, entry in aitypes do` to `for k, entry in
pairs(aitypes) do` to properly iterate over the table with both key and value
pairs.
In `@lua/ui/lobby/lobby_modules/autobalance.lua`:
- Around line 463-470: The repeat-until loop starting with ranNum =
math.random(1, table.getn(factionNames)) can infinitely loop if all faction
names are exhausted (all slots in nameSlotsTaken[playerFaction] are populated).
Add a safeguard by either checking if available slots remain before entering the
loop, or adding a maximum iteration counter to break out gracefully if no unused
names are available. Reference the condition
nameSlotsTaken[playerFaction][ranNum] == nil to determine when an available slot
exists.
In `@lua/ui/lobby/lobby_modules/createui.lua`:
- Around line 1483-1484: The inequality operator `!=` is used on line 1483 in
the conditional expression checking manual teams, but standard Lua syntax
requires `~=` for inequality comparisons. Replace both occurrences of `!=` in
the manualTeams condition (comparing numPlayersTeam1 != teamSize and
numPlayersTeam2 != teamSize) with the correct `~=` operator to match Lua syntax
standards.
In `@lua/ui/lobby/lobby_modules/hostutils.lua`:
- Around line 569-575: The function IsModAvailable is being called within the
UpdateMods() function in hostutils.lua but is not available in the scope of that
file since it's defined only in lobby.lua. To fix this, you need to inject
IsModAvailable as a dependency from lobby.lua into hostutils.lua. Add
IsModAvailable to the _syncHostUtils() function call in lobby.lua to pass it as
an injected function, then ensure it is declared in the injected functions list
at the top of hostutils.lua and properly initialized in the appropriate Init
function so that UpdateMods() can access and call IsModAvailable when checking
if mods are available.
In `@lua/ui/lobby/lobby_modules/launchlogic.lua`:
- Line 502: The assignment of gameInfo.AdaptiveMap from scenarioInfo.AdaptiveMap
on line 502 will fail if scenarioInfo is nil, which occurs when the conditional
block defining scenarioInfo (based on gameInfo.GameOptions.ScenarioFile) is not
executed. Add a nil check before this assignment to verify that scenarioInfo
exists before attempting to access scenarioInfo.AdaptiveMap. Only assign
gameInfo.AdaptiveMap if scenarioInfo is not nil.
In `@lua/ui/lobby/lobby_modules/maputils.lua`:
- Around line 186-214: In the manual auto-teams mode block where AutoTeams ==
'manual', there is a nil access risk at lines 209-211. The condition on line 189
can be satisfied even when gameInfo.PlayerOptions[slot] is nil (for open
unoccupied slots) because of the or clause checking TeamSpawn != 'fixed'. Add a
guard to ensure gameInfo.PlayerOptions[slot] is not nil before attempting to
access gameInfo.PlayerOptions[slot]['Team'] and calling SetSlotInfo, otherwise
the code will error when clicking an empty slot with TeamSpawn set to a
non-fixed value.
In `@lua/ui/lobby/lobby_modules/messagehandlers.lua`:
- Around line 304-306: The function FindRehostSlotForID is currently being
called as a global function in the messagehandlers.lua file, but for consistency
with the module's dependency injection pattern, it should be explicitly injected
like the similar functions FindSlotForID and FindObserverSlotForID. Add
FindRehostSlotForID to the dependencies table that is passed to
MessageHandlers.Init(), then update the call to FindRehostSlotForID in
messagehandlers.lua to use the injected dependency instead of the global
function reference.
- Around line 387-391: The SystemMessage handler is calling PrintSystemMessage
directly as a global function instead of using dependency injection. Update the
module's InitB function to accept PrintSystemMessage as a parameter (similar to
how SendSystemMessage, SendPersonalSystemMessage, and AddChatText are injected),
and then modify the Handle function within the SystemMessage table to use the
injected PrintSystemMessage instead of accessing it globally.
- Around line 374-377: The `availableMods` variable is used in the `Handle`
function within the `SetAvailableMods` message handler but is never assigned
from the dependencies object passed to the module during initialization. To fix
this, add an assignment statement `availableMods = deps.availableMods` in either
the `InitA` or `InitB` function so that the variable is properly captured from
the dependencies before it is accessed in the handler function.
In `@lua/ui/lobby/lobby_modules/optionsdialog.lua`:
- Around line 96-101: In the elseif block that handles the case where modNum
equals 0 and modNumUI is greater than 0, the singular form check on line 98 is
using the wrong variable. Change the condition from `modNum == 1` to `modNumUI
== 1` so that the singular "UI Mod" text is correctly used when there is exactly
one UI mod instead of checking a variable that is guaranteed to be zero in this
branch.
In `@lua/ui/lobby/lobby_modules/slotmenu.lua`:
- Line 104: The generic for loop iterating over menuData[stateKey][hostKey] is
missing an iterator function. Since this table appears to be a sequential array
of action keys, wrap the table reference with the ipairs() iterator function to
properly iterate over the indexed values. Change the for loop to use
ipairs(menuData[stateKey][hostKey]) instead of iterating directly over the table
without an iterator.
---
Nitpick comments:
In `@lua/ui/lobby/lobby_modules/chathandler.lua`:
- Around line 58-60: The nested property access chain in the line assigning
nameColor from skins lacks nil guards and will crash if GetLocalPlayerData()
returns nil, if AsTable() returns nil, or if the Faction property is nil. Add
defensive nil checks for each step of the chain: first verify
GetLocalPlayerData() is not nil, then verify the result of AsTable() is not nil,
and finally verify that the Faction property exists and is not nil before
attempting to access the skins array and retrieve the fontColor property. Only
assign nameColor and textColor if all intermediate values are valid.
In `@lua/ui/lobby/lobby_modules/cpubenchmark.lua`:
- Around line 93-101: The CPUText closure accesses
GUI.slots[slotNumber].CPUSpeedBar.CPUActualValue without verifying that
GUI.slots[slotNumber] exists, which could cause errors if the slot structure is
modified after initial creation. Add a nil guard to check if
GUI.slots[slotNumber] exists before attempting to access the nested CPUSpeedBar
and CPUActualValue properties; if the slot is nil, return the fallback LOC('<LOC
lobui_0458>UnKnown') message to ensure the function remains safe.
- Around line 149-152: Add nil guards before accessing GUI.rerunBenchmark and
its label property. Before the Disable() call on GUI.rerunBenchmark at line 149
and before accessing GUI.rerunBenchmark.label for the SetText() call at line
151, add checks to verify that GUI.rerunBenchmark exists and that
GUI.rerunBenchmark.label exists. This makes the code more resilient to cases
where these UI elements may not be fully initialized or available during the UI
lifecycle.
In `@lua/ui/lobby/lobby_modules/createui.lua`:
- Around line 1789-1791: The if false then block containing the
CreateSnowFlakes(GUI) function call is dead code that will never execute. Either
remove this entire block entirely if the snowflake feature is no longer needed,
or if this is an intentional seasonal or debug feature that should be enabled at
certain times, replace the false condition with a meaningful comment explaining
when and how this feature should be activated (such as a configuration flag,
seasonal check, or debug mode variable).
In `@lua/ui/lobby/lobby_modules/hostutils.lua`:
- Around line 497-510: The SendArmySettingsToServer function uses a hardcoded
magic number MAXSLOT with value 16 instead of leveraging the injected
LobbyComm.maxPlayerSlots constant. Replace the local MAXSLOT variable
declaration with a direct reference to LobbyComm.maxPlayerSlots in the for loop
condition to ensure consistency and eliminate the duplicate constant definition.
In `@lua/ui/lobby/lobby_modules/maputils.lua`:
- Around line 160-174: The variable requestedFaction is assigned the result of
GetSanitisedLastFaction() but is never used in the subsequent code. Remove the
assignment of requestedFaction on the line where it gets assigned from
GetSanitisedLastFaction(), as it serves no purpose in the current logic and only
clutters the code. If this faction information is actually needed by
HostUtils.ConvertObserverToPlayer, pass requestedFaction as a parameter to that
function call instead.
In `@lua/ui/lobby/lobby_modules/optionsdialog.lua`:
- Around line 313-334: Remove the unnecessary `if true then` wrapper and its
corresponding `end` statement from the snowflakes option block. The condition
always evaluates to true and serves no purpose. Keep all the content inside
intact, including the `currentSnowFlakesCount` variable initialization, the
`slider_SnowFlakes_Count_TEXT` text creation, the `slider_SnowFlakes_Count`
slider initialization, and the `OnValueChanged` callback function. Simply
unindent the code and remove both the opening and closing conditional
statements.
🪄 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: dafdcfac-0dc6-4088-8637-7b977898e921
📒 Files selected for processing (13)
lua/ui/lobby/lobby.lualua/ui/lobby/lobby_modules/aiutils.lualua/ui/lobby/lobby_modules/autobalance.lualua/ui/lobby/lobby_modules/chathandler.lualua/ui/lobby/lobby_modules/cpubenchmark.lualua/ui/lobby/lobby_modules/createui.lualua/ui/lobby/lobby_modules/hostutils.lualua/ui/lobby/lobby_modules/launchlogic.lualua/ui/lobby/lobby_modules/maputils.lualua/ui/lobby/lobby_modules/messagehandlers.lualua/ui/lobby/lobby_modules/optionsdialog.lualua/ui/lobby/lobby_modules/pingutils.lualua/ui/lobby/lobby_modules/slotmenu.lua
| function GetAIPlayerData(name, AIPersonality, slot) | ||
| local AIColor | ||
| -- gets the color of the player/AI occupying the slot directly prior if available | ||
| for i = 1, LobbyComm.maxPlayerSlots do | ||
| if gameInfo.PlayerOptions[i].StartSpot == slot then | ||
| if IsColorFree(gameInfo.PlayerOptions[i].PlayerColor, slot) then | ||
| AIColor = gameInfo.PlayerOptions[i].PlayerColor | ||
| end | ||
| break | ||
| end | ||
| end | ||
| if not AIColor then | ||
| AIColor = GetAvailableColor() | ||
| end | ||
|
|
||
| -- retrieve properties from AI table | ||
| ---@type AILobbyProperties | nil | ||
| local aiLobbyProperties = nil | ||
| for k, entry in aitypes do | ||
| if entry.key == AIPersonality then | ||
| aiLobbyProperties = entry | ||
| end | ||
| end | ||
| local iRating = ComputeAIRating(gameInfo.GameOptions, aiLobbyProperties) | ||
|
|
||
| return PlayerData( | ||
| { | ||
| OwnerID = hostID, | ||
| PlayerName = name, | ||
| Ready = true, | ||
| Human = false, | ||
| AIPersonality = AIPersonality, | ||
| PlayerColor = AIColor, | ||
| ArmyColor = AIColor, | ||
|
|
||
| PL = iRating, | ||
| MEAN = iRating, | ||
| DEV = 0, | ||
|
|
||
| -- keep track of the AI lobby properties for easier access | ||
| AILobbyProperties = aiLobbyProperties, | ||
| } | ||
| ) | ||
| end |
There was a problem hiding this comment.
Missing pairs() iterator in generic for loop.
Line 154 uses for k, entry in aitypes without an iterator function. This should use pairs(aitypes).
🐛 Proposed fix
---@type AILobbyProperties | nil
local aiLobbyProperties = nil
- for k, entry in aitypes do
+ for k, entry in pairs(aitypes) do
if entry.key == AIPersonality then
aiLobbyProperties = entry
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function GetAIPlayerData(name, AIPersonality, slot) | |
| local AIColor | |
| -- gets the color of the player/AI occupying the slot directly prior if available | |
| for i = 1, LobbyComm.maxPlayerSlots do | |
| if gameInfo.PlayerOptions[i].StartSpot == slot then | |
| if IsColorFree(gameInfo.PlayerOptions[i].PlayerColor, slot) then | |
| AIColor = gameInfo.PlayerOptions[i].PlayerColor | |
| end | |
| break | |
| end | |
| end | |
| if not AIColor then | |
| AIColor = GetAvailableColor() | |
| end | |
| -- retrieve properties from AI table | |
| ---@type AILobbyProperties | nil | |
| local aiLobbyProperties = nil | |
| for k, entry in aitypes do | |
| if entry.key == AIPersonality then | |
| aiLobbyProperties = entry | |
| end | |
| end | |
| local iRating = ComputeAIRating(gameInfo.GameOptions, aiLobbyProperties) | |
| return PlayerData( | |
| { | |
| OwnerID = hostID, | |
| PlayerName = name, | |
| Ready = true, | |
| Human = false, | |
| AIPersonality = AIPersonality, | |
| PlayerColor = AIColor, | |
| ArmyColor = AIColor, | |
| PL = iRating, | |
| MEAN = iRating, | |
| DEV = 0, | |
| -- keep track of the AI lobby properties for easier access | |
| AILobbyProperties = aiLobbyProperties, | |
| } | |
| ) | |
| end | |
| function GetAIPlayerData(name, AIPersonality, slot) | |
| local AIColor | |
| -- gets the color of the player/AI occupying the slot directly prior if available | |
| for i = 1, LobbyComm.maxPlayerSlots do | |
| if gameInfo.PlayerOptions[i].StartSpot == slot then | |
| if IsColorFree(gameInfo.PlayerOptions[i].PlayerColor, slot) then | |
| AIColor = gameInfo.PlayerOptions[i].PlayerColor | |
| end | |
| break | |
| end | |
| end | |
| if not AIColor then | |
| AIColor = GetAvailableColor() | |
| end | |
| -- retrieve properties from AI table | |
| ---@type AILobbyProperties | nil | |
| local aiLobbyProperties = nil | |
| for k, entry in pairs(aitypes) do | |
| if entry.key == AIPersonality then | |
| aiLobbyProperties = entry | |
| end | |
| end | |
| local iRating = ComputeAIRating(gameInfo.GameOptions, aiLobbyProperties) | |
| return PlayerData( | |
| { | |
| OwnerID = hostID, | |
| PlayerName = name, | |
| Ready = true, | |
| Human = false, | |
| AIPersonality = AIPersonality, | |
| PlayerColor = AIColor, | |
| ArmyColor = AIColor, | |
| PL = iRating, | |
| MEAN = iRating, | |
| DEV = 0, | |
| -- keep track of the AI lobby properties for easier access | |
| AILobbyProperties = aiLobbyProperties, | |
| } | |
| ) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/aiutils.lua` around lines 136 - 179, The generic
for loop iterating over the aitypes table in the GetAIPlayerData function is
missing the required iterator function. Change the loop statement from `for k,
entry in aitypes do` to `for k, entry in pairs(aitypes) do` to properly iterate
over the table with both key and value pairs.
| local ranNum | ||
| repeat | ||
| ranNum = math.random(1, table.getn(factionNames)) | ||
| until nameSlotsTaken[playerFaction][ranNum] == nil | ||
| nameSlotsTaken[playerFaction][ranNum] = true | ||
| player.PlayerName = factionNames[ranNum] .. " (" .. player.PlayerName .. ")" | ||
| end | ||
| end |
There was a problem hiding this comment.
Potential infinite loop if all faction names are exhausted.
If the number of AI players of a single faction exceeds the number of available names for that faction, the repeat-until loop will never terminate.
Consider adding a safeguard:
🛡️ Proposed defensive fix
for index, player in gameInfo.PlayerOptions do
if not player.Human then
local playerFaction = player.Faction
local factionNames = aiNames[FactionData.Factions[playerFaction].Key]
local ranNum
+ local attempts = 0
+ local maxAttempts = table.getn(factionNames) * 2
repeat
ranNum = math.random(1, table.getn(factionNames))
- until nameSlotsTaken[playerFaction][ranNum] == nil
+ attempts = attempts + 1
+ until nameSlotsTaken[playerFaction][ranNum] == nil or attempts >= maxAttempts
nameSlotsTaken[playerFaction][ranNum] = true
player.PlayerName = factionNames[ranNum] .. " (" .. player.PlayerName .. ")"
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local ranNum | |
| repeat | |
| ranNum = math.random(1, table.getn(factionNames)) | |
| until nameSlotsTaken[playerFaction][ranNum] == nil | |
| nameSlotsTaken[playerFaction][ranNum] = true | |
| player.PlayerName = factionNames[ranNum] .. " (" .. player.PlayerName .. ")" | |
| end | |
| end | |
| local ranNum | |
| local attempts = 0 | |
| local maxAttempts = table.getn(factionNames) * 2 | |
| repeat | |
| ranNum = math.random(1, table.getn(factionNames)) | |
| attempts = attempts + 1 | |
| until nameSlotsTaken[playerFaction][ranNum] == nil or attempts >= maxAttempts | |
| nameSlotsTaken[playerFaction][ranNum] = true | |
| player.PlayerName = factionNames[ranNum] .. " (" .. player.PlayerName .. ")" | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/autobalance.lua` around lines 463 - 470, The
repeat-until loop starting with ranNum = math.random(1,
table.getn(factionNames)) can infinitely loop if all faction names are exhausted
(all slots in nameSlotsTaken[playerFaction] are populated). Add a safeguard by
either checking if available slots remain before entering the loop, or adding a
maximum iteration counter to break out gracefully if no unused names are
available. Reference the condition nameSlotsTaken[playerFaction][ranNum] == nil
to determine when an available slot exists.
| if (manualTeams and (numPlayersTeam1 != teamSize or numPlayersTeam2 != teamSize)) | ||
| or (not manualTeams and (table.getn(sortedTeam1Slots) < teamSize or table.getn(sortedTeam2Slots) < teamSize)) then |
There was a problem hiding this comment.
Same != operator issue as in messagehandlers.lua.
Line 1483 uses != which Luacheck flags as invalid Lua syntax. This is the same issue as flagged in messagehandlers.lua. Standard Lua uses ~= for inequality. If SupCom Lua supports != as an extension, this is fine; otherwise both conditions on line 1483 need to use ~=.
🐛 Proposed fix (if != is not supported)
- if (manualTeams and (numPlayersTeam1 != teamSize or numPlayersTeam2 != teamSize))
- or (not manualTeams and (table.getn(sortedTeam1Slots) < teamSize or table.getn(sortedTeam2Slots) < teamSize)) then
+ if (manualTeams and (numPlayersTeam1 ~= teamSize or numPlayersTeam2 ~= teamSize))
+ or (not manualTeams and (table.getn(sortedTeam1Slots) < teamSize or table.getn(sortedTeam2Slots) < teamSize)) then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (manualTeams and (numPlayersTeam1 != teamSize or numPlayersTeam2 != teamSize)) | |
| or (not manualTeams and (table.getn(sortedTeam1Slots) < teamSize or table.getn(sortedTeam2Slots) < teamSize)) then | |
| if (manualTeams and (numPlayersTeam1 ~= teamSize or numPlayersTeam2 ~= teamSize)) | |
| or (not manualTeams and (table.getn(sortedTeam1Slots) < teamSize or table.getn(sortedTeam2Slots) < teamSize)) then |
🧰 Tools
🪛 Luacheck (1.2.0)
[error] 1483-1483: expected ')' near '!'
(E011)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/createui.lua` around lines 1483 - 1484, The
inequality operator `!=` is used on line 1483 in the conditional expression
checking manual teams, but standard Lua syntax requires `~=` for inequality
comparisons. Replace both occurrences of `!=` in the manualTeams condition
(comparing numPlayersTeam1 != teamSize and numPlayersTeam2 != teamSize) with the
correct `~=` operator to match Lua syntax standards.
| for modId, _ in selectedSimMods do | ||
| if IsModAvailable(modId) then | ||
| newmods[modId] = true | ||
| else | ||
| table.insert(missingmods, modId) | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for IsModAvailable definition to understand where it comes from
ast-grep --pattern 'function IsModAvailable($_) { $$$ }'
rg -n 'IsModAvailable\s*=' --type=lua
rg -n 'function\s+IsModAvailable' --type=luaRepository: FAForever/fa
Length of output: 120
🏁 Script executed:
# Check how hostutils.lua is structured and what's injected
head -100 lua/ui/lobby/lobby_modules/hostutils.luaRepository: FAForever/fa
Length of output: 3568
🏁 Script executed:
# Find where hostutils.lua is imported/initialized
rg -n 'hostutils' --type=lua | head -20Repository: FAForever/fa
Length of output: 239
🏁 Script executed:
# Check the InitA and InitB structure mentioned in the scratchpad
rg -n 'InitA\|InitB' lua/ui/lobby/lobby_modules/hostutils.lua | head -20Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
# Get the full InitB function and any module exports
sed -n '55,150p' lua/ui/lobby/lobby_modules/hostutils.luaRepository: FAForever/fa
Length of output: 3769
🏁 Script executed:
# Look at the UpdateMods function where IsModAvailable is called
sed -n '560,580p' lua/ui/lobby/lobby_modules/hostutils.luaRepository: FAForever/fa
Length of output: 704
🏁 Script executed:
# Check how lobby.lua initializes HostUtils with dependencies
rg -n 'HostUtils\.Init\|InitHostUtils' lua/ui/lobby/lobby.lua -A 10Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
# Search for where HostUtils Init is called in lobby.lua
rg -n 'HostUtils_Module\.Init\|HostUtils\.Init' lua/ui/lobby/lobby.lua -A 5Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
# Check if IsModAvailable is passed in the Init call or if it's a global function
rg -n 'IsModAvailable' lua/ui/lobby/lobby.lua -B 2 -A 2Repository: FAForever/fa
Length of output: 179
🏁 Script executed:
# Search more carefully for how HostUtils is initialized
rg -n 'HostUtils' lua/ui/lobby/lobby.lua -B 2 -A 5Repository: FAForever/fa
Length of output: 10877
🏁 Script executed:
# Check if IsModAvailable might be injected differently or if there's another definition
rg -n 'IsModAvailable' lua/ui/lobby/ --type=lua -lRepository: FAForever/fa
Length of output: 120
🏁 Script executed:
# Look at the complete UpdateMods function to understand its full scope
sed -n '540,600p' lua/ui/lobby/lobby_modules/hostutils.luaRepository: FAForever/fa
Length of output: 2157
🏁 Script executed:
# Check the complete SyncModuleDeps function to see all dependencies passed to HostUtils_Module.Init
rg -n 'SyncModuleDeps' lua/ui/lobby/lobby.lua -A 100 | head -150Repository: FAForever/fa
Length of output: 7034
🏁 Script executed:
# Look for InitHostUtils and check what happens there
sed -n '2661,2670p' lua/ui/lobby/lobby.luaRepository: FAForever/fa
Length of output: 347
🏁 Script executed:
# Get the full SyncModuleDeps function definition
sed -n '1135,1250p' lua/ui/lobby/lobby.luaRepository: FAForever/fa
Length of output: 5363
🏁 Script executed:
# Also check if there's any global access to IsModAvailable or if it's somehow accessible through upvalues
grep -n 'IsModAvailable' lua/ui/lobby/lobby_modules/hostutils.luaRepository: FAForever/fa
Length of output: 98
🏁 Script executed:
# Find the _syncHostUtils function
rg -n '_syncHostUtils' lua/ui/lobby/lobby.lua -A 50Repository: FAForever/fa
Length of output: 5412
IsModAvailable function must be injected into hostutils.lua.
The function IsModAvailable(modId) is called on line 570 in UpdateMods(), but it is not included in the dependencies injected via _syncHostUtils() in lobby.lua. Since IsModAvailable is defined locally in lobby.lua (line 364), it is not accessible to hostutils.lua. This will cause a runtime error when UpdateMods() is called on the host.
Add IsModAvailable = IsModAvailable to the _syncHostUtils() initialization in lobby.lua (around line 1449) and to both the injected functions list (top of hostutils.lua) and the appropriate Init function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/hostutils.lua` around lines 569 - 575, The
function IsModAvailable is being called within the UpdateMods() function in
hostutils.lua but is not available in the scope of that file since it's defined
only in lobby.lua. To fix this, you need to inject IsModAvailable as a
dependency from lobby.lua into hostutils.lua. Add IsModAvailable to the
_syncHostUtils() function call in lobby.lua to pass it as an injected function,
then ensure it is declared in the injected functions list at the top of
hostutils.lua and properly initialized in the appropriate Init function so that
UpdateMods() can access and call IsModAvailable when checking if mods are
available.
| UIUtil.setEnabled(GUI.factionSelector, false) | ||
| end | ||
|
|
||
| gameInfo.AdaptiveMap = scenarioInfo.AdaptiveMap |
There was a problem hiding this comment.
Potential nil dereference: scenarioInfo may be undefined.
Line 502 accesses scenarioInfo.AdaptiveMap outside the conditional block (lines 354-477) that assigns scenarioInfo. If gameInfo.GameOptions.ScenarioFile is nil or empty, scenarioInfo remains nil and this line will error.
🐛 Proposed fix
- gameInfo.AdaptiveMap = scenarioInfo.AdaptiveMap
+ gameInfo.AdaptiveMap = scenarioInfo and scenarioInfo.AdaptiveMap📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gameInfo.AdaptiveMap = scenarioInfo.AdaptiveMap | |
| gameInfo.AdaptiveMap = scenarioInfo and scenarioInfo.AdaptiveMap |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/launchlogic.lua` at line 502, The assignment of
gameInfo.AdaptiveMap from scenarioInfo.AdaptiveMap on line 502 will fail if
scenarioInfo is nil, which occurs when the conditional block defining
scenarioInfo (based on gameInfo.GameOptions.ScenarioFile) is not executed. Add a
nil check before this assignment to verify that scenarioInfo exists before
attempting to access scenarioInfo.AdaptiveMap. Only assign gameInfo.AdaptiveMap
if scenarioInfo is not nil.
| if argv.isRehost then | ||
| local rehostSlot = FindRehostSlotForID(data.SenderID) or 0 | ||
| if rehostSlot ~= 0 and gameInfo.PlayerOptions[rehostSlot] then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for FindRehostSlotForID definition
rg -n 'FindRehostSlotForID' --type=lua -C2Repository: FAForever/fa
Length of output: 1180
🏁 Script executed:
# Check the context around InitA/InitB in messagehandlers.lua
rg -n 'InitA|InitB' lua/ui/lobby/lobby_modules/messagehandlers.lua -A 5 -B 2Repository: FAForever/fa
Length of output: 1085
🏁 Script executed:
# Check the top of messagehandlers.lua for dependency setup
head -100 lua/ui/lobby/lobby_modules/messagehandlers.luaRepository: FAForever/fa
Length of output: 3521
🏁 Script executed:
# Check if FindRehostSlotForID is defined as a global function in lobby.lua
rg -n 'function FindRehostSlotForID' lua/ui/lobby/lobby.lua -B 5 -A 10Repository: FAForever/fa
Length of output: 435
🏁 Script executed:
# Search for where messagehandlers is required/loaded
rg -n 'messagehandlers|MessageHandlers' lua/ui/lobby/lobby.lua | head -20Repository: FAForever/fa
Length of output: 569
🏁 Script executed:
# Check if FindRehostSlotForID is referenced in the deps being passed to Init
rg -n 'FindRehostSlotForID' lua/ui/lobby/lobby.luaRepository: FAForever/fa
Length of output: 163
🏁 Script executed:
# Check what deps are passed to MessageHandlers.Init in _syncMessageHandlers
rg -n '_syncMessageHandlers' lua/ui/lobby/lobby.lua -A 50 | head -80Repository: FAForever/fa
Length of output: 4092
Remove injection claim; this is a pattern inconsistency, not a runtime error.
FindRehostSlotForID is defined as a global function in lobby.lua (line 947), so it will be accessible from messagehandlers.lua without explicit injection. However, this creates an inconsistency: similar functions like FindSlotForID and FindObserverSlotForID are explicitly injected in the dependency table passed to MessageHandlers.Init() at line 1336. For consistency with the module's dependency injection pattern, FindRehostSlotForID should be added to the deps table.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/messagehandlers.lua` around lines 304 - 306, The
function FindRehostSlotForID is currently being called as a global function in
the messagehandlers.lua file, but for consistency with the module's dependency
injection pattern, it should be explicitly injected like the similar functions
FindSlotForID and FindObserverSlotForID. Add FindRehostSlotForID to the
dependencies table that is passed to MessageHandlers.Init(), then update the
call to FindRehostSlotForID in messagehandlers.lua to use the injected
dependency instead of the global function reference.
| Handle = function(data) | ||
| availableMods[data.SenderID] = data.Mods | ||
| HostUtils.UpdateMods(data.SenderID, data.Name) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify availableMods is a global or check if it should be injected
rg -n 'availableMods\s*=' --type=lua | head -20Repository: FAForever/fa
Length of output: 503
🏁 Script executed:
cat -n lua/ui/lobby/lobby_modules/messagehandlers.lua | sed -n '360,390p'Repository: FAForever/fa
Length of output: 1214
🏁 Script executed:
rg -n "local\s+\w+\s*=\s*function\|function\s+\w+\(" lua/ui/lobby/lobby_modules/messagehandlers.lua | grep -A5 -B5 "SetAvailableMods"Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
rg -n "SetAvailableMods" lua/ui/lobby/lobby_modules/messagehandlers.lua -A 15 -B 5Repository: FAForever/fa
Length of output: 586
🏁 Script executed:
# Check how dependencies are injected into messagehandlers module
head -100 lua/ui/lobby/lobby_modules/messagehandlers.lua | tail -50Repository: FAForever/fa
Length of output: 2266
🏁 Script executed:
rg -n "availableMods" lua/ui/lobby/lobby_modules/messagehandlers.luaRepository: FAForever/fa
Length of output: 113
🏁 Script executed:
# Get the complete InitA and InitB functions
sed -n '1,150p' lua/ui/lobby/lobby_modules/messagehandlers.lua | grep -n "availableMods\|function Init"Repository: FAForever/fa
Length of output: 140
🏁 Script executed:
# Check the full extent of InitA and InitB
awk '/^local function InitA/,/^end/ {print NR": "$0}' lua/ui/lobby/lobby_modules/messagehandlers.luaRepository: FAForever/fa
Length of output: 1129
🏁 Script executed:
sed -n '82,115p' lua/ui/lobby/lobby_modules/messagehandlers.luaRepository: FAForever/fa
Length of output: 1547
🏁 Script executed:
# Check how messagehandlers is initialized in lobby.lua
rg -n "messagehandlers" lua/ui/lobby/lobby.lua | head -20Repository: FAForever/fa
Length of output: 141
🏁 Script executed:
# Find where MessageHandlers.Init is called
rg -n "MessageHandlers\.Init" lua/ui/lobby/lobby.lua -A 30 -B 5Repository: FAForever/fa
Length of output: 1976
🏁 Script executed:
sed -n '1336,1430p' lua/ui/lobby/lobby.lua | grep -n "availableMods"Repository: FAForever/fa
Length of output: 119
🏁 Script executed:
# Get complete _syncMessageHandlers function
sed -n '1335,1450p' lua/ui/lobby/lobby.luaRepository: FAForever/fa
Length of output: 6130
availableMods is not assigned from dependencies in messagehandlers.lua.
The variable availableMods is passed in the dependency object when MessageHandlers.Init is called from lobby.lua:1414, but the messagehandlers module does not assign it in either the InitA or InitB functions. This means line 375 in the SetAvailableMods handler will attempt to access an undefined availableMods, resulting in a nil access error.
Add availableMods = deps.availableMods to either the InitA or InitB function in messagehandlers.lua to resolve this.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/messagehandlers.lua` around lines 374 - 377, The
`availableMods` variable is used in the `Handle` function within the
`SetAvailableMods` message handler but is never assigned from the dependencies
object passed to the module during initialization. To fix this, add an
assignment statement `availableMods = deps.availableMods` in either the `InitA`
or `InitB` function so that the variable is properly captured from the
dependencies before it is accessed in the handler function.
| SystemMessage = { | ||
| Handle = function(data) | ||
| PrintSystemMessage(data.Id, data.Args) | ||
| end | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for PrintSystemMessage definition
rg -n 'function\s+PrintSystemMessage' --type=lua
rg -n 'PrintSystemMessage\s*=' --type=luaRepository: FAForever/fa
Length of output: 128
🏁 Script executed:
cat -n lua/ui/lobby/lobby_modules/messagehandlers.lua | head -50Repository: FAForever/fa
Length of output: 1661
🏁 Script executed:
cat -n lua/ui/lobby/lobby_modules/messagehandlers.lua | grep -A 10 -B 10 "function InitA\|function InitB"Repository: FAForever/fa
Length of output: 2106
🏁 Script executed:
cat -n lua/ui/lobby/lobby.lua | sed -n '2010,2030p'Repository: FAForever/fa
Length of output: 753
🏁 Script executed:
rg -n "PrintSystemMessage" lua/ui/lobby/lobby_modules/messagehandlers.lua -B 5 -A 5Repository: FAForever/fa
Length of output: 310
🏁 Script executed:
cat -n lua/ui/lobby/lobby_modules/messagehandlers.lua | sed -n '82,120p'Repository: FAForever/fa
Length of output: 2066
🏁 Script executed:
rg -n "PrintSystemMessage" lua/ui/lobby/lobby.lua -B 2 -A 2Repository: FAForever/fa
Length of output: 341
🏁 Script executed:
# Check if PrintSystemMessage is accessible as a global or if it needs to be injected
rg -n "local PrintSystemMessage\|_G\[.*PrintSystemMessage\|function PrintSystemMessage" lua/ui/lobby/Repository: FAForever/fa
Length of output: 38
PrintSystemMessage function is not included in the dependency injection pattern.
The function PrintSystemMessage is called on line 389 but is not injected via InitB deps like other similar functions (SendSystemMessage, SendPersonalSystemMessage, AddChatText). While PrintSystemMessage is globally accessible from lobby.lua, it should be injected for consistency with the module's dependency injection pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/messagehandlers.lua` around lines 387 - 391, The
SystemMessage handler is calling PrintSystemMessage directly as a global
function instead of using dependency injection. Update the module's InitB
function to accept PrintSystemMessage as a parameter (similar to how
SendSystemMessage, SendPersonalSystemMessage, and AddChatText are injected), and
then modify the Handle function within the SystemMessage table to use the
injected PrintSystemMessage instead of accessing it globally.
| elseif modNum == 0 and modNumUI > 0 then | ||
| modStr = modNumUI..' UI Mods' | ||
| if modNum == 1 then | ||
| modStr = modNumUI..' UI Mod' | ||
| end | ||
| end |
There was a problem hiding this comment.
Bug: Wrong variable checked for singular form.
Line 98 checks modNum == 1 but we're in the modNum == 0 branch, so this condition is always false. The singular "UI Mod" text will never be used.
🐛 Proposed fix
elseif modNum == 0 and modNumUI > 0 then
modStr = modNumUI..' UI Mods'
- if modNum == 1 then
+ if modNumUI == 1 then
modStr = modNumUI..' UI Mod'
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elseif modNum == 0 and modNumUI > 0 then | |
| modStr = modNumUI..' UI Mods' | |
| if modNum == 1 then | |
| modStr = modNumUI..' UI Mod' | |
| end | |
| end | |
| elseif modNum == 0 and modNumUI > 0 then | |
| modStr = modNumUI..' UI Mods' | |
| if modNumUI == 1 then | |
| modStr = modNumUI..' UI Mod' | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/optionsdialog.lua` around lines 96 - 101, In the
elseif block that handles the case where modNum equals 0 and modNumUI is greater
than 0, the singular form check on line 98 is using the wrong variable. Change
the condition from `modNum == 1` to `modNumUI == 1` so that the singular "UI
Mod" text is correctly used when there is exactly one UI mod instead of checking
a variable that is guaranteed to be zero in this branch.
| return nil | ||
| end | ||
|
|
||
| for _, key in menuData[stateKey][hostKey] do |
There was a problem hiding this comment.
Missing pairs() iterator in generic for loop.
Line 104 iterates menuData[stateKey][hostKey] without an iterator function. This should use ipairs() since the table appears to be a sequential array of action keys.
🐛 Proposed fix
- for _, key in menuData[stateKey][hostKey] do
+ for _, key in ipairs(menuData[stateKey][hostKey]) do🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/slotmenu.lua` at line 104, The generic for loop
iterating over menuData[stateKey][hostKey] is missing an iterator function.
Since this table appears to be a sequential array of action keys, wrap the table
reference with the ipairs() iterator function to properly iterate over the
indexed values. Change the for loop to use ipairs(menuData[stateKey][hostKey])
instead of iterating directly over the table without an iterator.
TimMasalme
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lua/ui/lobby/lobby_modules/chathandler.lua`:
- Around line 59-62: The code on line 62 accesses skins[factionKey].fontColor
without first checking if skins[factionKey] exists, which will cause an error if
a faction has no corresponding skin entry (such as modded factions). Add a nil
guard to check that skins[factionKey] exists before accessing its fontColor
property. You can either add the check directly in the assignment statement for
factionKey, or better yet, cache the skin lookup in a local variable (like
skinTable) and use that to safely access fontColor, avoiding redundant import
calls and making the code more readable.
🪄 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: 0901eeb7-690d-463f-b2b8-25f303bbb6df
📒 Files selected for processing (5)
lua/ui/lobby/lobby_modules/chathandler.lualua/ui/lobby/lobby_modules/cpubenchmark.lualua/ui/lobby/lobby_modules/createui.lualua/ui/lobby/lobby_modules/maputils.lualua/ui/lobby/lobby_modules/optionsdialog.lua
💤 Files with no reviewable changes (1)
- lua/ui/lobby/lobby_modules/maputils.lua
🚧 Files skipped from review as they are similar to previous changes (3)
- lua/ui/lobby/lobby_modules/optionsdialog.lua
- lua/ui/lobby/lobby_modules/createui.lua
- lua/ui/lobby/lobby_modules/cpubenchmark.lua
| local localData = GetLocalPlayerData() | ||
| local localTable = localData and localData:AsTable() | ||
| local factionKey = localTable and FACTION_NAMES[localTable.Faction] | ||
| nameColor = factionKey and import("/lua/skins/skins.lua").skins[factionKey].fontColor or nameColor |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate FACTION_NAMES definitions/usages:"
rg -n --type=lua -C2 '\bFACTION_NAMES\b'
echo
echo "Locate skins.lua and inspect available keys/usages:"
fd -i 'skins.lua' | while read -r f; do
echo "---- $f ----"
rg -n -C2 'skins\[' "$f"
done
echo
echo "Find direct skin fontColor dereferences (potentially unsafe):"
rg -n --type=lua -C2 'skins\[[^]]+\]\.fontColor'Repository: FAForever/fa
Length of output: 9328
🏁 Script executed:
# Get file size and read skins.lua to see actual skin key definitions
wc -l lua/skins/skins.lua
echo "---"
head -100 lua/skins/skins.luaRepository: FAForever/fa
Length of output: 3832
🏁 Script executed:
# Search for where skins table is populated with keys
rg -n --type=lua 'skins\s*=\s*\{' lua/skins/skins.lua -A 50Repository: FAForever/fa
Length of output: 2596
🏁 Script executed:
# Search for "random" skin entry specifically
rg -n --type=lua 'random' lua/skins/skins.luaRepository: FAForever/fa
Length of output: 125
🏁 Script executed:
# Read skins.lua from line 130 onwards to see if seraphim and other entries exist
sed -n '130,220p' lua/skins/skins.luaRepository: FAForever/fa
Length of output: 5311
🏁 Script executed:
# Search specifically for seraphim skin entry
rg -n 'seraphim\s*=' lua/skins/skins.lua -A 5Repository: FAForever/fa
Length of output: 373
🏁 Script executed:
# Check if uef has fontColor
rg -n 'uef\s*=\s*{' lua/skins/skins.lua -A 20 | grep -i fontcolorRepository: FAForever/fa
Length of output: 142
🏁 Script executed:
# Check seraphim fontColor
rg -n 'seraphim\s*=\s*{' lua/skins/skins.lua -A 30 | grep -i fontcolorRepository: FAForever/fa
Length of output: 152
🏁 Script executed:
# Check random fontColor
rg -n 'random\s*=\s*{' lua/skins/skins.lua -A 30 | grep -i fontcolorRepository: FAForever/fa
Length of output: 148
🏁 Script executed:
# Check how localData.Faction is populated/validated
rg -n 'localData\.Faction\|GetLocalPlayerData' lua/ui/lobby/lobby_modules/chathandler.lua -B 3 -A 3Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
# Read the uef skin section
sed -n '59,68p' lua/skins/skins.lua | grep -i fontcolor
echo "---"
# Read seraphim section
sed -n '111,132p' lua/skins/skins.lua | grep -i fontcolor
echo "---"
# Read random section
sed -n '133,153p' lua/skins/skins.lua | grep -i fontcolorRepository: FAForever/fa
Length of output: 96
🏁 Script executed:
# Search how Faction value is obtained and if it's validated before use
rg -n 'Faction' lua/ui/lobby/lobby_modules/chathandler.lua -B 5 -A 2Repository: FAForever/fa
Length of output: 606
🏁 Script executed:
# Check GetLocalPlayerData to understand what values Faction can have
fd -i 'playerdata' | xargs rg -l 'Faction' 2>/dev/null | head -5Repository: FAForever/fa
Length of output: 89
🏁 Script executed:
# Check when skins flattening happens and if it's guaranteed to run before chathandler use
rg -n 'assimilate\|default chain' lua/skins/skins.lua -B 3 -A 5Repository: FAForever/fa
Length of output: 38
🏁 Script executed:
# Verify all faction skins have fontColor after checking default chain logic
# Read full uef entry
sed -n '59,68p' lua/skins/skins.lua
echo "---full seraphim---"
sed -n '111,132p' lua/skins/skins.luaRepository: FAForever/fa
Length of output: 1578
🏁 Script executed:
# Check if localTable.Faction could be anything other than integer 1-5
rg -n 'Faction' lua/ui/lobby/data/playerdata.lua -B 2 -A 2Repository: FAForever/fa
Length of output: 382
Add nil guard before dereferencing skins[factionKey].fontColor on line 62.
Line 62 dereferences skins[factionKey].fontColor without checking if skins[factionKey] exists. While all five standard factions (uef, aeon, cybran, seraphim, random) have skin entries in the skins table (with properties inherited via the default chain), the code does not guard against modded factions or Faction values outside the standard set. If a faction lacks a corresponding skin entry, this will error when posting chat messages.
Change:
nameColor = factionKey and import("/lua/skins/skins.lua").skins[factionKey] and import("/lua/skins/skins.lua").skins[factionKey].fontColor or nameColorOr cache the import and skin lookup to avoid redundant calls:
local skinTable = factionKey and import("/lua/skins/skins.lua").skins[factionKey]
nameColor = skinTable and skinTable.fontColor or nameColor🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lua/ui/lobby/lobby_modules/chathandler.lua` around lines 59 - 62, The code on
line 62 accesses skins[factionKey].fontColor without first checking if
skins[factionKey] exists, which will cause an error if a faction has no
corresponding skin entry (such as modded factions). Add a nil guard to check
that skins[factionKey] exists before accessing its fontColor property. You can
either add the check directly in the assignment statement for factionKey, or
better yet, cache the skin lookup in a local variable (like skinTable) and use
that to safely access fontColor, avoiding redundant import calls and making the
code more readable.
- CreateUI, OptionsDialog, ChatHandler, etc. extracted into separate modules - Each module uses Init(deps) pattern with rawset injection - Avoids 32-upvalue limit and strict-global guard - InitLobbyComm deliberately kept in lobby.lua (see comment)
|
I'd say this actually makes worse maintainability, everything stayed tightly coupled exactly as it was before, its just spread among more files now, which makes is harder to track and know what affects what. |
2 participants
Summary
lobby.luahad grown to ~7 400 lines, making it very hard to navigate,review, and test individual features. This PR extracts its major subsystems
into focused modules under
lua/ui/lobby/lobby_modules/, reducing the mainfile to ~2 400 lines while keeping it as the single orchestration entry point.
Total lines before: ~7 400 in one file
Total lines after: ~2 400 (
lobby.lua) + ~5 000 across 12 modulesExtracted modules
createui.luaCreateUI,CreateSlotsUI, faction selector)hostutils.luamessagehandlers.lualobbyCommmessage handlerslaunchlogic.luaautobalance.luaoptionsdialog.luamaputils.luachathandler.luacpubenchmark.luapingutils.luaaiutils.luaslotmenu.luaArchitecture
Each module follows the same
Init(deps)injection pattern alreadyestablished in the codebase:
lobby.luaimports each module and owns all shared state (locals).SyncModuleDeps()inlobby.luacalls every module'sInit{...}to(re-)inject the current values before any cross-module call.
rawset(getfenv(1), k, v)is used insideInitto bypass SupCom'sstrict-global guard without hitting the 32-upvalue-per-function limit.
InitLobbyCommand itslobbyComm.*callbacks are deliberately notextracted — they directly reassign
lobby.lua's own locals (lobbyComm,GUI,hostID,localPlayerID,localPlayerName) andKeepAliveThreadFuncloops on
while lobbyComm do, which requires a live upvalue. Extracting themwould add significant complexity for no functional gain.
Testing
Tested in a local FAF session (host + join). No regressions observed in:
Summary by CodeRabbit