Conversation
|
|
WalkthroughAdds a Changes
Sequence DiagramsequenceDiagram
participant User
participant HostLobbyModal
participant LobbyPlayerView
participant Server_GameConfig
participant GamePreviewBuilder
User->>HostLobbyModal: select "Random" map
HostLobbyModal->>HostLobbyModal: set useRandomMap = true
HostLobbyModal->>LobbyPlayerView: .isRandomMap = true
LobbyPlayerView->>LobbyPlayerView: render "?" placeholders
HostLobbyModal->>Server_GameConfig: send Partial<GameConfig> (useRandomMap)
Server_GameConfig->>GamePreviewBuilder: include useRandomMap in ExternalGameInfo
GamePreviewBuilder->>GamePreviewBuilder: if useRandomMap then hide map thumbnail and prefer RandomMap.webp
GamePreviewBuilder-->>User: embed/preview without specific map spoiler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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}
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)
src/server/GamePreviewBuilder.ts (1)
202-206:⚠️ Potential issue | 🟠 MajorEmbed title still reveals the randomly selected map name.
The PR hides the map image and nation count when
useRandomMapis true, but thetitleat Line 204 still renders"FFA on World"(or whatever the actual map is). On platforms like Discord, WhatsApp, etc., the embed title is visible and would spoil the random map selection.Consider masking the map name in the title too:
Proposed fix
+ const displayMap = useRandomMap ? "Random" : map; + const title = isFinished ? `${mode ?? "Game"} on ${map ?? "Unknown Map"}${gameTypeLabel}` - : mode && map - ? `${mode} on ${map}${gameTypeLabel}` + : mode && displayMap + ? `${mode} on ${displayMap}${gameTypeLabel}` : "OpenFront Game";
🤖 Fix all issues with AI agents
In `@src/core/Schemas.ts`:
- Line 178: Run Prettier to fix the formatting in src/core/Schemas.ts and commit
the changes; specifically reformat the function/constructor block that currently
shows ") { }" so it matches project Prettier rules (you can run prettier --write
on src/core/Schemas.ts or the repo root), then stage and push the formatted
file.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
seems to sill be exposed in the joinlobbymodal
|
|
Also inside the websocket - many tampermonkey scripts read this stuff |
|
Ah, will fix the join lobby modal. Didn't catch that. Should the desired behavior have it obfuscated from the websocket? |
I think singleplayer modal selects the map on start, maybe we could do something like this? not sure |
| const displayCount = isNationsTeam | ||
| ? this.isRandomMap | ||
| ? "?" | ||
| : effectiveNationCount | ||
| : preview.players.length; | ||
|
|
||
| const maxTeamSize = isNationsTeam | ||
| ? this.isRandomMap | ||
| ? "?" | ||
| : effectiveNationCount | ||
| : this.teamMaxSize; |
There was a problem hiding this comment.
can you turn this into regular if statements, nested ternary are hard to reason about
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/client/components/LobbyPlayerView.ts (2)
56-57:isRandomMaptrigger inwillUpdatecauses a needless team recompute.
isCompactMap(line 56) correctly triggers recomputation becausegetEffectiveNationCount()→getCompactMapNationCount()reads it. However, neithercomputeTeamPreview()norgetTeamList()readsisRandomMap. The intended pattern forwillUpdateis to only recheck changed properties that feed an expensive derived computation. Therender()cycle already fires on any reactive-property change, soisRandomMapis visible in the template without being listed here.♻️ Suggested fix
- changedProperties.has("isCompactMap") || - changedProperties.has("isRandomMap") + changedProperties.has("isCompactMap")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/LobbyPlayerView.ts` around lines 56 - 57, The willUpdate check is over-triggering team recompute because it includes isRandomMap even though computeTeamPreview() and getTeamList() don't read it; update the willUpdate condition that uses changedProperties.has(...) so it only checks isCompactMap (and any other properties that feed getEffectiveNationCount()/getCompactMapNationCount()) and remove changedProperties.has("isRandomMap") so that computeTeamPreview()/getTeamList() are only recomputed when their true inputs change.
77-80:getEffectiveNationCount()is called twice perrender().
renderTeamCardalready caches it in aconst(line 187). Apply the same pattern inrender()to match the existing convention and keep it DRY.♻️ Suggested refactor
render() { + const effectiveNationCount = this.getEffectiveNationCount(); return html` ... - ${this.isRandomMap ? "?" : this.getEffectiveNationCount()} - ${this.isRandomMap || this.getEffectiveNationCount() !== 1 + ${this.isRandomMap ? "?" : effectiveNationCount} + ${this.isRandomMap || effectiveNationCount !== 1 ? translateText("host_modal.nation_players") : translateText("host_modal.nation_player")}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/LobbyPlayerView.ts` around lines 77 - 80, In render() you're calling getEffectiveNationCount() twice; match the pattern used in renderTeamCard by caching its result in a const (e.g., const effectiveNationCount = this.getEffectiveNationCount()) and then use effectiveNationCount wherever getEffectiveNationCount() is currently invoked (including the conditional that renders translateText("host_modal.nation_players") vs translateText("host_modal.nation_player")), leaving this.isRandomMap checks unchanged.
🤖 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/client/components/LobbyPlayerView.ts`:
- Line 41: The JoinLobbyModal is not passing the isRandomMap boolean into the
<lobby-player-view>, causing the nation count to show a number instead of "?";
update the JoinLobbyModal render where <lobby-player-view> is instantiated to
include the property .isRandomMap=${this.gameConfig?.useRandomMap ?? false}
(same pattern used in HostLobbyModal), ensuring the LobbyPlayerView.@property({
type: Boolean }) isRandomMap is set correctly.
---
Nitpick comments:
In `@src/client/components/LobbyPlayerView.ts`:
- Around line 56-57: The willUpdate check is over-triggering team recompute
because it includes isRandomMap even though computeTeamPreview() and
getTeamList() don't read it; update the willUpdate condition that uses
changedProperties.has(...) so it only checks isCompactMap (and any other
properties that feed getEffectiveNationCount()/getCompactMapNationCount()) and
remove changedProperties.has("isRandomMap") so that
computeTeamPreview()/getTeamList() are only recomputed when their true inputs
change.
- Around line 77-80: In render() you're calling getEffectiveNationCount() twice;
match the pattern used in renderTeamCard by caching its result in a const (e.g.,
const effectiveNationCount = this.getEffectiveNationCount()) and then use
effectiveNationCount wherever getEffectiveNationCount() is currently invoked
(including the conditional that renders
translateText("host_modal.nation_players") vs
translateText("host_modal.nation_player")), leaving this.isRandomMap checks
unchanged.
| @property({ type: Number }) nationCount: number = 0; | ||
| @property({ type: Boolean }) disableNations: boolean = false; | ||
| @property({ type: Boolean }) isCompactMap: boolean = false; | ||
| @property({ type: Boolean }) isRandomMap: boolean = false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all TypeScript files related to join/lobby/modal
echo "=== All TypeScript files containing 'Join' or 'join' ==="
fd --type=f -e ts | rg -i "join" | head -20
echo ""
echo "=== Check for JoinLobbyModal specifically ==="
fd --type=f -e ts | xargs rg -l "JoinLobbyModal" 2>/dev/null
echo ""
echo "=== All modal-related files ==="
fd --type=f -e ts | rg -i "modal" | head -20Repository: openfrontio/OpenFrontIO
Length of output: 953
🏁 Script executed:
#!/bin/bash
# Search for nation count display in join/lobby context
echo "=== Files containing 'nationCount' or 'nation_count' ==="
rg -l "nationCount|nation_count" --type=ts
echo ""
echo "=== Context around nationCount in those files ==="
rg -n -C5 "nationCount|nation_count" --type=ts | head -100Repository: openfrontio/OpenFrontIO
Length of output: 7854
🏁 Script executed:
#!/bin/bash
# Check LobbyPlayerView.ts structure and uses
echo "=== LobbyPlayerView.ts content ==="
cat -n src/client/components/LobbyPlayerView.ts | head -100
echo ""
echo "=== Check where LobbyPlayerView is used ==="
rg -n "lobby-player-view|LobbyPlayerView" --type=ts -A2 -B2Repository: openfrontio/OpenFrontIO
Length of output: 7018
🏁 Script executed:
#!/bin/bash
# Search for join lobby modal and its nation count display
echo "=== Search for useRandomMap references ==="
rg -n "useRandomMap" --type=ts -C3
echo ""
echo "=== Search for isRandomMap references ==="
rg -n "isRandomMap" --type=ts -C3Repository: openfrontio/OpenFrontIO
Length of output: 17152
Pass isRandomMap to lobby-player-view in join lobby modal.
The <lobby-player-view> component in JoinLobbyModal.ts (line 130-142) is missing the .isRandomMap property. When a random map is selected, the nation count display shows a number instead of "?" like in the host modal.
Add: .isRandomMap=${this.gameConfig?.useRandomMap ?? false}
This matches the pattern already used in HostLobbyModal.ts (line 324).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/LobbyPlayerView.ts` at line 41, The JoinLobbyModal is
not passing the isRandomMap boolean into the <lobby-player-view>, causing the
nation count to show a number instead of "?"; update the JoinLobbyModal render
where <lobby-player-view> is instantiated to include the property
.isRandomMap=${this.gameConfig?.useRandomMap ?? false} (same pattern used in
HostLobbyModal), ensuring the LobbyPlayerView.@property({ type: Boolean })
isRandomMap is set correctly.
4 participants
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #3211
Description:
When randomly selecting a map, the game accidentally exposes information about the underlying map selected through the nation count at the bottom (thanks @FloPinguin) and the attached embed image when the link is distributed. This is now obscured with a "?" for the nation count, and the image is updated to be the "RandomMap.webp"
Please complete the following:
As seen in the bottom of the image below, this is with the normal-sized world map selected, where it says "61 NATIONS"
This is with the random map selected, where the number is now hidden with "? NATIONS"
Please put your Discord username so you can be contacted if a bug or regression is found:
ProDCG