Fix: prevent 2-3 public lobbies showing the same map/max players/number of teams#3567
Fix: prevent 2-3 public lobbies showing the same map/max players/number of teams#3567VariableVince wants to merge 9 commits intomainfrom
Conversation
…or Team games) number of teams. It was relatively quite frequently happening that one map showed up in two or even three lobbies at the same time. Same for teams games, the Special Mix lobby would have 42 Humans vs 42 Nations and the Teams lobby would show the exact same configuration. Or there would be two lobbies with 125 players.
VariableVince
added
the
Refactor
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughMapPlaylist now provides Changes
Sequence DiagramsequenceDiagram
participant MLS as MasterLobbyService
participant MP as MapPlaylist
participant IS as InUseStore
participant WK as Worker
MLS->>IS: collect in-use keys (map / teams / maxPlayers)
MLS->>MP: gameConfigNotInUse(type, predicate)
loop up to 6 attempts
MP->>MP: ensurePlaylistPopulated(type) / peekNextMap(type)
MP->>MP: buildConfig(type, map) or buildSpecialConfig(map)
MP->>IS: predicate(config) -> isInUse?
alt not in use
MP->>MP: consumeNextMap(type) — remove chosen map
MP-->>MLS: return GameConfig
else in use
MP->>MP: cycleNextMap(type) — rotate or regenerate
MP-->>MP: retry
end
end
MLS->>IS: recordInUse(selected config)
MLS->>WK: send createGame(selected config)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🧹 Nitpick comments (1)
src/server/MapPlaylist.ts (1)
152-171: Method name does not match behavior after fallback.After 6 attempts, this method returns a config regardless of whether
isInUse(config)is true. The namegameConfigNotInUsesuggests a guarantee that is not provided. Consider renaming to something clearer likegameConfigAvoidingCollisionortryGetUniqueGameConfig, or add a doc comment explaining the fallback behavior.The
do { ... } while (true)with an internal return works, but you could also express the limit more directly:♻️ Suggested rewrite for clarity
public async gameConfigNotInUse( type: PublicGameType, isInUse: (config: GameConfig) => boolean, ): Promise<GameConfig> { const maxAttempts = 6; - let attempts = 0; - - do { + for (let attempts = 0; attempts < maxAttempts; attempts++) { const map = this.tryFirstMap(type); const config = await this.buildConfig(type, map); - attempts++; - if (!isInUse(config) || attempts >= maxAttempts) { + if (!isInUse(config) || attempts === maxAttempts - 1) { this.useFirstMap(type); return config; } this.firstMapToLast(type); - } while (true); + } + // Unreachable, but satisfies TypeScript + throw new Error("Unexpected: maxAttempts exhausted without return"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/MapPlaylist.ts` around lines 152 - 171, The method gameConfigNotInUse incorrectly promises a config "not in use" but after maxAttempts returns whichever config was last built even if isInUse(config) is true; either rename the method to something clearer (e.g., tryGetUniqueGameConfig or gameConfigAvoidingCollision) and update all callers, or change the implementation to explicitly reflect the fallback: loop up to maxAttempts (use a for loop over attempts), call tryFirstMap -> buildConfig and check isInUse(config), returning immediately if not in use; if loop exits without finding a free config, call useFirstMap(type) once and return the last built config; reference the functions tryFirstMap, buildConfig, useFirstMap, firstMapToLast, and the isInUse callback when making the change and add a short doc comment describing the fallback behavior.
🤖 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/server/MasterLobbyService.ts`:
- Around line 151-157: The loop only calls recordInUse on lobbies[0], so other
existing lobbies per type (e.g., lobbies[1], lobbies[2]) are never marked in
usedMaps/usedTeamTypes/usedMaxPlayers; change the iteration over lobbyTypes to
iterate every lobby in lobbiesByType[type] (e.g., for each const lobby of
lobbies) and call recordInUse(lobby.gameConfig) when gameConfig is present,
ensuring all existing lobbies are recorded and preventing collisions; update
references to lobbyTypes, lobbiesByType, and recordInUse accordingly.
---
Nitpick comments:
In `@src/server/MapPlaylist.ts`:
- Around line 152-171: The method gameConfigNotInUse incorrectly promises a
config "not in use" but after maxAttempts returns whichever config was last
built even if isInUse(config) is true; either rename the method to something
clearer (e.g., tryGetUniqueGameConfig or gameConfigAvoidingCollision) and update
all callers, or change the implementation to explicitly reflect the fallback:
loop up to maxAttempts (use a for loop over attempts), call tryFirstMap ->
buildConfig and check isInUse(config), returning immediately if not in use; if
loop exits without finding a free config, call useFirstMap(type) once and return
the last built config; reference the functions tryFirstMap, buildConfig,
useFirstMap, firstMapToLast, and the isInUse callback when making the change and
add a short doc comment describing the fallback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21ca9ff7-de42-40e1-80b0-fde667a38950
📒 Files selected for processing (2)
src/server/MapPlaylist.tssrc/server/MasterLobbyService.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
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/server/MapPlaylist.ts`:
- Around line 438-440: firstMapToLast currently moves the first element using a
raw push(this.playlists[type].shift()!) which can introduce consecutive
duplicates; instead, after tryFirstMap(type) remove the head value and attempt
to reinsert it using the same non-consecutive rule (call
addNextMapNonConsecutive(type, removedMap) or equivalent) and if that
reinsertion fails, rebuild the tail using generateNewPlaylist(type) (or a
dedicated rebuildTail method) so the non-consecutive invariant maintained by
generateNewPlaylist is preserved; update firstMapToLast to locate the head via
this.playlists[type].shift(), attempt addNextMapNonConsecutive and fallback to
generateNewPlaylist on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90f4cfe3-d305-486d-ac9c-774d40e4f2b9
📒 Files selected for processing (1)
src/server/MapPlaylist.ts
github-project-automation
bot
moved this from Final Review
to Development
in OpenFront Release Management
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/server/MapPlaylist.ts (1)
438-440:⚠️ Potential issue | 🟠 MajorRaw
push()here can recreate back-to-back maps.
generateNewPlaylist()avoids reusing any of the last five maps, but Line 440 bypasses that rule. A valid cycle like[A, ..., A]becomes[..., A, A]after one rejection, so this retry path can reintroduce the same-map problem this PR is trying to reduce. Please reinsert the rejected map withaddNextMapNonConsecutive()and rebuild the tail when that is not possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/MapPlaylist.ts` around lines 438 - 440, The current firstMapToLast method moves the rejected first map back into playlists[type] with a raw push which can create back-to-back duplicates; instead call addNextMapNonConsecutive(type, rejectedMap) to reinsert the rejected map so non-consecutive rules are respected, and if addNextMapNonConsecutive cannot place it (returns false or throws), rebuild the tail for that playlist by invoking generateNewPlaylist(type) (or the existing tail-rebuild helper) after tryFirstMap(type) to guarantee the non-consecutive constraint is preserved; keep tryFirstMap(type) as the rejection check and replace the playlists[type].push(this.playlists[type].shift()!) step with this safer reinsertion + fallback rebuild logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/MapPlaylist.ts`:
- Around line 438-440: The current firstMapToLast method moves the rejected
first map back into playlists[type] with a raw push which can create
back-to-back duplicates; instead call addNextMapNonConsecutive(type,
rejectedMap) to reinsert the rejected map so non-consecutive rules are
respected, and if addNextMapNonConsecutive cannot place it (returns false or
throws), rebuild the tail for that playlist by invoking
generateNewPlaylist(type) (or the existing tail-rebuild helper) after
tryFirstMap(type) to guarantee the non-consecutive constraint is preserved; keep
tryFirstMap(type) as the rejection check and replace the
playlists[type].push(this.playlists[type].shift()!) step with this safer
reinsertion + fallback rebuild logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4408923b-d973-493e-bc2b-b72e6ceafcc5
📒 Files selected for processing (1)
src/server/MapPlaylist.ts
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/server/MapPlaylist.ts (1)
446-450:⚠️ Potential issue | 🟠 MajorRebuild the fallback tail with the current suffix, not a standalone playlist.
If
addNextMapNonConsecutive()fails here, appending a freshgenerateNewPlaylist()can still put one of the current last five maps right at the boundary, so the non-consecutive rule comes back later in the cycle. This also drops the rejectedmapfrom the current rotation.♻️ One way to keep the invariant intact
private cycleNextMap(type: PublicGameType): void { const map = this.consumeNextMap(type); - if (!this.addNextMapNonConsecutive(this.playlists[type], [map])) { - this.playlists[type].push(...this.generateNewPlaylist(type)); + if (this.addNextMapNonConsecutive(this.playlists[type], [map])) { + return; + } + + const rebuilt = [...this.playlists[type], map]; + this.playlists[type] = []; + while (rebuilt.length > 0) { + if (!this.addNextMapNonConsecutive(this.playlists[type], rebuilt)) { + this.playlists[type] = this.generateNewPlaylist(type); + break; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/MapPlaylist.ts` around lines 446 - 450, The current cycleNextMap drops the consumed map and appends a fresh generateNewPlaylist() which can reintroduce a recent map at the boundary; instead rebuild the fallback tail using the actual current suffix (including the rejected map) so addNextMapNonConsecutive can validate against the real last-N maps. Change the failure branch in cycleNextMap: compute the suffix = last N entries of this.playlists[type] concatenated with [map] (use the same window size that addNextMapNonConsecutive enforces), then generate or extend a replacement tail that is validated by addNextMapNonConsecutive(suffix, tail) (or call addNextMapNonConsecutive(this.playlists[type], tail) while ensuring tail was built from that suffix); loop/regenerate until addNextMapNonConsecutive returns true so the rejected map remains in rotation and the non-consecutive invariant holds (use functions consumeNextMap, addNextMapNonConsecutive, generateNewPlaylist as reference points when implementing).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/MapPlaylist.ts`:
- Around line 446-450: The current cycleNextMap drops the consumed map and
appends a fresh generateNewPlaylist() which can reintroduce a recent map at the
boundary; instead rebuild the fallback tail using the actual current suffix
(including the rejected map) so addNextMapNonConsecutive can validate against
the real last-N maps. Change the failure branch in cycleNextMap: compute the
suffix = last N entries of this.playlists[type] concatenated with [map] (use the
same window size that addNextMapNonConsecutive enforces), then generate or
extend a replacement tail that is validated by addNextMapNonConsecutive(suffix,
tail) (or call addNextMapNonConsecutive(this.playlists[type], tail) while
ensuring tail was built from that suffix); loop/regenerate until
addNextMapNonConsecutive returns true so the rejected map remains in rotation
and the non-consecutive invariant holds (use functions consumeNextMap,
addNextMapNonConsecutive, generateNewPlaylist as reference points when
implementing).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f815bd24-579c-44e3-a5e6-eb6eb55d1f17
📒 Files selected for processing (2)
src/server/MapPlaylist.tssrc/server/MasterLobbyService.ts
There was a problem hiding this comment.
This seems a bit over complicated to me. could we just do something like:
activeMaps => new Set(lobbies.map(l => l.map))
while(true) {
const config => mapPlaylist.config()
if(!activeMaps.has(config.map)) {
continue
}
2 participants
Description:
Try to prevent two or three of the public lobbies having the same map/number of players/number of teams. Six tries max before it just goes with what is has. We could make it e.g. three tries if six is two much.
Because it is (relatively) quite frequently happening that public lobbies have:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33