Open
Conversation
lL1l1
added
area: code style
Contributor
📝 WalkthroughWalkthroughAdds a new public function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/keymap/selectedinfo.lua (1)
47-58:⚠️ Potential issue | 🟡 MinorPotential nil access on first-iteration cycle detection.
If the focus chain loops back to the original unit on the first iteration (i.e.,
unit:GetFocus():GetEntityId() == unit:GetEntityId()), theninfo.focushasn't been assigned yet andinfo.focus.focus = nilon line 50 will error.🐛 Proposed fix
while focus do local id = focus:GetEntityId() if visited[id] then - info.focus.focus = nil + if info.focus then + info.focus.focus = nil + end break end visited[id] = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/keymap/selectedinfo.lua` around lines 47 - 58, The loop can detect a cycle on the first iteration where info.focus is still nil; replace the unconditional assignment info.focus.focus = nil with a nil-safe write: check if info.focus then set info.focus.focus = nil else set info.focus = nil, so the cycle-break path doesn't attempt to index a nil. Update the code around the while loop using the symbols focus, visited, info.focus, and focusingInfo/GetUnitRolloverInfo to apply this conditional assignment before breaking.
🧹 Nitpick comments (1)
lua/keymap/selectedinfo.lua (1)
19-21: Missing@paramannotation forskipFocus.The
skipFocusparameter is not documented. Add an annotation for completeness.📝 Proposed fix
---@param unit UserUnit +---@param skipFocus? boolean ---@return RolloverInfo function GetUnitRolloverInfo(unit, skipFocus)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/keymap/selectedinfo.lua` around lines 19 - 21, Add a missing LuaDoc `@param` annotation for the skipFocus parameter on the GetUnitRolloverInfo function: update the comment block above GetUnitRolloverInfo to include a line like "@param skipFocus boolean? — skip focusing logic when true" (or use the appropriate type and short description used in this project) so the function signature documents both unit and skipFocus consistently with existing annotations.
🤖 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/keymap/selectedinfo.lua`:
- Around line 79-90: Remove the dead local variables by deleting the unused
import/assignment and table: drop the local Prefs =
import("/lua/user/prefs.lua") and the subsequent local options =
Prefs.GetFromCurrentProfile('options') if Prefs is only used for options, and
remove the unused local selectionOverlay table (symbols: Prefs, options,
selectionOverlay); if Prefs is used elsewhere keep the import but remove only
the options assignment and the selectionOverlay block to eliminate dead code and
leftover backwards-compatibility comments.
---
Outside diff comments:
In `@lua/keymap/selectedinfo.lua`:
- Around line 47-58: The loop can detect a cycle on the first iteration where
info.focus is still nil; replace the unconditional assignment info.focus.focus =
nil with a nil-safe write: check if info.focus then set info.focus.focus = nil
else set info.focus = nil, so the cycle-break path doesn't attempt to index a
nil. Update the code around the while loop using the symbols focus, visited,
info.focus, and focusingInfo/GetUnitRolloverInfo to apply this conditional
assignment before breaking.
---
Nitpick comments:
In `@lua/keymap/selectedinfo.lua`:
- Around line 19-21: Add a missing LuaDoc `@param` annotation for the skipFocus
parameter on the GetUnitRolloverInfo function: update the comment block above
GetUnitRolloverInfo to include a line like "@param skipFocus boolean? — skip
focusing logic when true" (or use the appropriate type and short description
used in this project) so the function signature documents both unit and
skipFocus consistently with existing annotations.
Comment on lines
+79
to
+90
| --#region Backwards compatibility | ||
| local Prefs = import("/lua/user/prefs.lua") | ||
| local options = Prefs.GetFromCurrentProfile('options') | ||
|
|
||
| local selectionOverlay = { | ||
| key = 'selection', | ||
| Label = "<LOC map_options_0006>Selection", | ||
| Pref = 'range_RenderSelected', | ||
| Type = 3, | ||
| Tooltip = "overlay_selection", | ||
| } | ||
| --#endregion |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if options and selectionOverlay are used in selectedinfo.lua
echo "=== Checking usage of 'options' in selectedinfo.lua ==="
rg -n '\boptions\b' lua/keymap/selectedinfo.lua
echo ""
echo "=== Checking usage of 'selectionOverlay' in selectedinfo.lua ==="
rg -n '\bselectionOverlay\b' lua/keymap/selectedinfo.luaRepository: FAForever/fa
Length of output: 265
Remove unused options and selectionOverlay variables.
Both variables are defined but never used in the file. The options variable on line 81 and selectionOverlay table on lines 83-89 appear only at their definition points and have no other references. Since they're local, they cannot provide backwards compatibility to external code. Remove these dead code assignments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lua/keymap/selectedinfo.lua` around lines 79 - 90, Remove the dead local
variables by deleting the unused import/assignment and table: drop the local
Prefs = import("/lua/user/prefs.lua") and the subsequent local options =
Prefs.GetFromCurrentProfile('options') if Prefs is only used for options, and
remove the unused local selectionOverlay table (symbols: Prefs, options,
selectionOverlay); if Prefs is used elsewhere keep the import but remove only
the options assignment and the selectionOverlay block to eliminate dead code and
leftover backwards-compatibility comments.
4z0t
reviewed
Mar 3, 2026
| info.tacticalSiloMaxStorageCount = missileInfo.tacticalSiloMaxStorageCount | ||
| info.tacticalSiloStorageCount = missileInfo.tacticalSiloStorageCount | ||
|
|
||
| info.customName = unit:GetCustomName(unit) |
Member
There was a problem hiding this comment.
Does it work? There was an engine bug that I fixed, but I remember @Garanas complained about it.
Contributor
Author
There was a problem hiding this comment.
It doesn't work, you're right.
Tested with selection:GetCustomName(nil) and sel[1]:GetCustomName(sel[2])
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/keymap/selectedinfo.lua (1)
49-51:⚠️ Potential issue | 🔴 CriticalFix cycle handling nil-dereference in focus traversal.
At Line 50,
info.focusis not guaranteed to exist. A self-referential focus can crash here. Use the current tail node (focusingInfo) instead.Proposed fix
if visited[id] then - info.focus.focus = nil + focusingInfo.focus = nil break end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/keymap/selectedinfo.lua` around lines 49 - 51, The cycle handling in the focus traversal can nil-dereference because info.focus may not exist; update the visited-check branch to operate on the current tail node (focusingInfo) instead of directly using info.focus: when visited[id] is true, set focusingInfo.focus = nil (or clear the focus on the current focusingInfo node after verifying it exists) and then break to stop traversal. Ensure you reference and null-check focusingInfo before mutating its .focus to avoid the self-referential crash.
🧹 Nitpick comments (1)
lua/keymap/selectedinfo.lua (1)
19-21: DocumentskipFocusin the function contract.
GetUnitRolloverInfo(unit, skipFocus)currently documents onlyunit. AddingskipFocusintent/behavior will make usage clearer and avoid guesswork.Proposed doc update
---@param unit UserUnit +---@param skipFocus? boolean -- when true, omit focus-chain expansion ---@return RolloverInfo function GetUnitRolloverInfo(unit, skipFocus)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lua/keymap/selectedinfo.lua` around lines 19 - 21, The function GetUnitRolloverInfo(unit, skipFocus) documents only the unit parameter; add documentation for the skipFocus parameter in the function contract by describing its type (boolean), default behavior, and effect on rollover behavior (e.g., when true, do not change or focus the UI/selection or suppress related focus side-effects), and include its return impact if any; update the function signature comment block above GetUnitRolloverInfo to include `@param` skipFocus and a short sentence about its intent and expected values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lua/keymap/selectedinfo.lua`:
- Around line 49-51: The cycle handling in the focus traversal can
nil-dereference because info.focus may not exist; update the visited-check
branch to operate on the current tail node (focusingInfo) instead of directly
using info.focus: when visited[id] is true, set focusingInfo.focus = nil (or
clear the focus on the current focusingInfo node after verifying it exists) and
then break to stop traversal. Ensure you reference and null-check focusingInfo
before mutating its .focus to avoid the self-referential crash.
---
Nitpick comments:
In `@lua/keymap/selectedinfo.lua`:
- Around line 19-21: The function GetUnitRolloverInfo(unit, skipFocus) documents
only the unit parameter; add documentation for the skipFocus parameter in the
function contract by describing its type (boolean), default behavior, and effect
on rollover behavior (e.g., when true, do not change or focus the UI/selection
or suppress related focus side-effects), and include its return impact if any;
update the function signature comment block above GetUnitRolloverInfo to include
`@param` skipFocus and a short sentence about its intent and expected values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d419cf00-0e9c-42f0-aff6-504647312db8
📒 Files selected for processing (2)
engine/User/UserUnit.lualua/keymap/selectedinfo.lua
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
2 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.
Description of the proposed changes
Testing done on the proposed changes
No warning from intellisense.
Checklist
Summary by CodeRabbit
New Features
Refactor
Documentation