Feature/random map hidden previews and nation count#4158
Open
noahschmal wants to merge 9 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a ChangesRandom Map Visibility Control
🎯 3 (Moderate) | ⏱️ ~25 minutes 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. 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: 3
🤖 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 `@resources/lang/en.json`:
- Line 522: Remove the duplicate "random" key in the "map" object so only a
single "random": "Random" remains; locate the repeated "random" entry in the
"map" section of resources/lang/en.json and delete the earlier occurrence (leave
the later declaration intact) to avoid duplicate JSON keys.
In `@src/client/components/LobbyPlayerView.ts`:
- Around line 89-95: The team preview still shows nation-derived counts even
when randomMap is true; update the render logic in LobbyPlayerView so any place
that uses effectiveNationCount-derived values (notably displayCount and
maxTeamSize used in the team preview) checks this.randomMap and instead renders
the masked placeholder (e.g., translateText("common.map_default") or the same
nation_players label) when randomMap is true; locate usages of displayCount and
maxTeamSize in the component (including the `${displayCount}/${maxTeamSize}`
expression) and replace them with conditional expressions that output the masked
text when this.randomMap is true.
In `@tests/server/GamePreviewRandomMap.test.ts`:
- Around line 20-58: Add a third test in GamePreviewRandomMap.test.ts to cover
the case where randomMap is omitted (undefined): create a test similar to the
"shows the concrete map for a normal lobby" one but call lobby({ gameMap:
"Europe", /* no randomMap */ gameType: "Private", gameMode: "FFA" }) when
invoking buildPreview, and assert that meta.image includes "europe"
(case-insensitive) and meta.title contains "Europe"; reference buildPreview and
lobby to locate where to add this new test case.
🪄 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: f8a60cf7-e859-41c1-bc98-8c9c423aa04c
📒 Files selected for processing (14)
resources/lang/en.jsonsrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/SinglePlayerModal.tssrc/client/components/FluentSlider.tssrc/client/components/GameConfigSettings.tssrc/client/components/LobbyPlayerView.tssrc/core/Schemas.tssrc/server/GamePreviewBuilder.tssrc/server/GameServer.tstests/RandomMapConfig.test.tstests/client/components/FluentSlider.test.tstests/server/GameLifecycle.test.tstests/server/GamePreviewRandomMap.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Selecting "Random" still resolves a concrete map client-side (so nation count and map loading work as today), but the choice now rides a `randomMap` flag on GameConfig. While the flag is set, all pre-start surfaces hide the concrete map so an invite link doesn't spoil it: - Invite embed (GamePreviewBuilder): generic RandomMap.webp image and a "Random Map" title instead of the real thumbnail/name. - Join screen + lobby player view: map shows "Random", and the nation count (which is per-map and would reveal the map) shows "Map default". - Host's nation slider: FluentSlider gains `hideDefaultValue` so the default position reads "Map default" without the revealing number. The server clears the flag at prestart, which reveals the (already resolved) map everywhere with no downstream special-casing. Applies to random lobbies only; concrete-map lobbies are unchanged. Tests cover the schema round-trip, prestart clearing the flag, updateGameConfig persisting it, and the embed hiding/showing the map.
Single-player uses the same game-config-settings nation slider, so pass hideDefaultValue when Random is selected — otherwise the default position shows the map's real nation count, revealing the randomly chosen map. No embed/lobby surfaces exist for single-player, so this is the only change.
Hiding the nation count number wasn't enough: the slider thumb still sat at value/max (the map's real nation count / 400), so the map could be inferred from the thumb's position. When a slider is at a hidden default (a Random lobby), render the thumb — and the fill bar and number editor — at the track center instead. Dragging or typing still reports the real input value, so the host can still override it.
Once you move a Random lobby's nation slider off the default, the real default count is hidden, so there's no way to drag back to it. Add a small "↺ Map default" reset button that appears only in that state (hideDefaultValue + moved off default); clicking it restores the default value, which re-parks the thumb at center and hides the button again.
- Remove the duplicate "map.random" en.json key (one already existed);
reuse the existing entry for translateText("map.random").
- Close the nation-count leak in team mode: the masked header wasn't
enough — team-card totals and team sizing also derive from the count.
Make effectiveNationCount return 0 for random lobbies so nothing
nation-derived reveals the map, and recompute the team preview when the
randomMap flag flips.
- Add a buildPreview test for randomMap omitted entirely (the common
normal-lobby case, since the field is optional).
- Add docstrings to the functions touched by this change to satisfy the
docstring-coverage threshold.
Add docstrings to willUpdate (LobbyPlayerView) and the lobby() test helper, the two functions in the latest changes that were still undocumented.
Add a docstring to the cdnBase mock arrow in the preview test — the last function the coverage scanner flagged.
noahschmal
force-pushed
the
feature/random-map-hidden
branch
from
385ccd1 to
dda3404
Compare
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
1 participant
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.
Add approved & assigned issue number here:
Resolves #3211
Description:
When a host selects the random map option, the map is resolved in the lobby and leaks into the nation count and preview embeds for the lobby. This PR fixes this by adding a randomMap flag on GameConfig that keeps it hidden on any visual surfaces. This also adjusts the nation slider to show "Map Default" with the fill in the middle of the slider for the random map, and adds a "↺ map default" button to reset it to default. The slider is set to the middle, obscuring which random map it is. The "↺ map default" button may be useful for all maps. Note that there is no provided preview of what a preview embed looks like since I can't access that when hosting locally, however, there is an associated test, and the image is the RandomMap.webp asset.
Default nation count in the middle with no number:
Slider moved from default, with reset to default visible:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
jetaviz