Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a clan-based team naming heuristic and integrates it into team assignment: computeClanTeamName determines a clan or coalition name per team; GameImpl uses it when adding players in numbered-team modes, resolves rename collisions, and applies safe renames to playerTeams and player→team mappings; tests added. Changes
Sequence DiagramsequenceDiagram
participant Game as GameImpl
participant Assign as TeamAssignment
participant Map as PlayerMappings
Game->>Game: Detect numbered-team mode
Game->>Map: Build team -> players reverse map
loop per team
Game->>Assign: computeClanTeamName(players)
Assign->>Assign: Filter humans, count clan tags
Assign->>Assign: Choose unanimous / majority / coalition / null
Assign-->>Game: Return candidate name or null
end
Game->>Game: Build rename map (exclude nulls & identical names)
Game->>Game: Iterative collision resolution vs existing names and other renames
Game->>Map: Apply renames to playerTeams and playerToTeam mappings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/game/GameImpl.ts (1)
216-218: Minor: Avoidas anycastThe cast works but is a bit loose. Since
Team = stringandObject.values(ColoredTeams)returnsstring[], you can avoid it:- const isNumberedTeams = !this.playerTeams.some((t) => - Object.values(ColoredTeams).includes(t as any), - ); + const coloredTeamValues = Object.values(ColoredTeams); + const isNumberedTeams = !this.playerTeams.some((t) => + coloredTeamValues.includes(t), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 216 - 218, The includes cast is too loose: in GameImpl.ts update the isNumberedTeams expression to remove the unnecessary "as any" by comparing the team value directly against the ColoredTeams values (e.g. const isNumberedTeams = !this.playerTeams.some(t => Object.values(ColoredTeams).includes(t));), or if needed explicitly type t as string/Team to satisfy TypeScript; reference symbols: isNumberedTeams, playerTeams, ColoredTeams.src/core/game/TeamAssignment.ts (1)
127-132: Consider limiting coalition name lengthThe PR objectives mention limiting hybrid names' character length to avoid UI/leaderboard overflow. Currently, long clan tags could produce names like
VERYLONGTAG1 / VERYLONGTAG2which might not fit well in the UI.Consider adding a max-length check, falling back to
nullwhen the coalition string is too long.💡 Possible length check
// Coalition: top two clans cover the majority of humans if (sorted.length >= 2) { const [secondTag, secondCount] = sorted[1]; if ((topCount + secondCount) / total > 0.5) { - return `${topTag} / ${secondTag}`; + const coalition = `${topTag} / ${secondTag}`; + const MAX_TEAM_NAME_LENGTH = 20; // adjust as needed + if (coalition.length <= MAX_TEAM_NAME_LENGTH) { + return coalition; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/TeamAssignment.ts` around lines 127 - 132, The coalition name can become too long (e.g., "VERYLONGTAG1 / VERYLONGTAG2"); update the logic in TeamAssignment where the coalition is formed (the block that computes `${topTag} / ${secondTag}` using sorted, topCount and secondCount) to enforce a maximum character length: define a COALITION_NAME_MAX_LENGTH constant and before returning the concatenated string check its length and return null if it exceeds the limit. Ensure you apply the check to the exact return site that currently returns `${topTag} / ${secondTag}` so long hybrid names are rejected consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/game/GameImpl.ts`:
- Around line 237-254: The collision check in GameImpl.ts only compares
renameMap.values() against themselves and misses conflicts where a newName
equals an existing team name that isn't being renamed; update the logic around
renameMap to also consider current team names in this.playerTeams (excluding the
team being renamed) by building a Set of existingNames from this.playerTeams
and, when iterating renameMap.entries(), drop the rename
(renameMap.delete(oldTeam)) if newName exists in existingNames or if it collides
with another rename as currently checked; keep the subsequent application to
this.playerTeams and playerToTeam unchanged.
---
Nitpick comments:
In `@src/core/game/GameImpl.ts`:
- Around line 216-218: The includes cast is too loose: in GameImpl.ts update the
isNumberedTeams expression to remove the unnecessary "as any" by comparing the
team value directly against the ColoredTeams values (e.g. const isNumberedTeams
= !this.playerTeams.some(t => Object.values(ColoredTeams).includes(t));), or if
needed explicitly type t as string/Team to satisfy TypeScript; reference
symbols: isNumberedTeams, playerTeams, ColoredTeams.
In `@src/core/game/TeamAssignment.ts`:
- Around line 127-132: The coalition name can become too long (e.g.,
"VERYLONGTAG1 / VERYLONGTAG2"); update the logic in TeamAssignment where the
coalition is formed (the block that computes `${topTag} / ${secondTag}` using
sorted, topCount and secondCount) to enforce a maximum character length: define
a COALITION_NAME_MAX_LENGTH constant and before returning the concatenated
string check its length and return null if it exceeds the limit. Ensure you
apply the check to the exact return site that currently returns `${topTag} /
${secondTag}` so long hybrid names are rejected consistently.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/game/GameImpl.tssrc/core/game/TeamAssignment.tstests/TeamAssignment.test.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
|
love this feature, reminds me of rocket league lol |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/game/GameImpl.ts`:
- Around line 241-251: The collision check for renames is flawed because
existingNames and newNames are computed once while renameMap is mutated; update
the logic to perform a fixed-point collision pass: repeatedly recompute
existingNames (teams not in renameMap) and newNames
(Array.from(renameMap.values())), iterate renameMap entries and remove any entry
whose newName collides with existingNames or appears more than once in newNames,
and repeat until a full pass makes no deletions, then apply the remaining
renames; reference renameMap, existingNames, newNames, and this.playerTeams when
locating the code to change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #3300
Description:
In games with 8+ teams (where teams are not named after territory colors), name teams after the clan tags majority when possible, fallback to numbered team names otherwise.
Screenshot taken from a duo in solo mode (I'm the only human in the team):
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
deshack_82603