Implement Map voting #3018
Draft
Implement Map voting #3018
+865
−240
Conversation
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
|
|
Contributor
WalkthroughAdds a map-voting feature: new MapVoteModal UI, local vote storage, client WebSocket vote plumbing, lobby UI integration, server-side vote collection/broadcast, and weighted, dynamic map selection in playlist generation. Changes
Sequence DiagramsequenceDiagram
participant User
participant Client
participant Storage as LocalStorage
participant WS as WebSocket
participant Server
participant Playlist
User->>Client: Open map-vote modal
Client->>Storage: loadStoredMapVotes()
Storage-->>Client: stored maps
Client->>Client: Render available maps + selection
User->>Client: Toggle maps
Client->>Storage: saveStoredMapVotes(maps)
alt User logged in
User->>Client: Submit vote
Client->>WS: Send `map_vote` (token + maps)
WS->>Server: Receive and verify vote
Server->>Server: Update per-user votes
Server->>Server: Aggregate weights -> mapVoteWeights
Server->>WS: Broadcast `map_vote_stats` / `map_vote_request`
WS-->>Client: Deliver stats/request
else User not logged in
Client->>Storage: Keep votes locally (no send)
end
Note over Server,Playlist: When scheduling public game
Server->>Playlist: gameConfig(mapVoteWeights)
Playlist->>Playlist: pickWeightedMap(using weights + recentMaps)
Playlist-->>Server: Return game config
Server->>WS: Broadcast new game config
Server->>Server: Clear votes and broadcast next `map_vote_request`
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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
🤖 Fix all issues with AI agents
In `@src/client/MapVoteModal.ts`:
- Around line 80-89: The map tile div is not keyboard-focusable; replace the
clickable <div> with a semantic <button> (or if keeping the element, add
tabindex="0" role="button" and keydown handlers) so keyboard users can activate
it; ensure the button calls toggleMapSelection(mapValue) on click and on
Enter/Space keydown, keep the existing classes (cursor-pointer,
transition-transform, active:scale-95), and add an accessible state attribute
such as aria-pressed={this.selectedMaps.has(mapValue)} and a clear label using
the same translateText(`map.${mapKey?.toLowerCase()}`) so map-display remains
accessible.
🧹 Nitpick comments (4)
src/client/Main.ts (1)
226-226: This property is scaffolding for a future feature and can be safely removed if not needed yet.
mapVoteModalis assigned from the DOM during initialization but never read afterward. The property follows the codebase pattern of preparing modal components in advance. If the map vote feature is not yet implemented, you can remove this property and lines 279-284 to reduce unused code. If this feature is planned, consider adding a comment to clarify the intent:// TODO: Implement map voting feature private mapVoteModal: MapVoteModal | null = null;src/client/MapVoteModal.ts (1)
14-18: Prefer composition over extending BaseModal.If possible, wrap a modal element instead of inheriting from BaseModal. This keeps the component smaller and easier to reuse. Please confirm inheritance is required here.
src/server/Master.ts (1)
83-85: Make vote weight configurable (avoid debug constant).A hardcoded
1e10will fully swamp base weights and is marked debug. Make this configurable with a sane default so tuning is easy.♻️ Proposed refactor
-const MAP_VOTE_WEIGHT = 10000000000; // for debug purposes, make votes very impactful 'w' +const MAP_VOTE_WEIGHT = + Number(process.env.MAP_VOTE_WEIGHT ?? 1000) || 1000;src/client/PublicLobby.ts (1)
82-96: Reduce duplicate auth + send logic.Both methods repeat the same auth steps. A small helper keeps this cleaner.
♻️ Suggested refactor
+ private async getAuthJwt(): Promise<string | null> { + if (!this.isLoggedIn) return null; + const auth = await userAuth(); + return auth ? auth.jwt : null; + } + private async sendStoredVotes() { - if (!this.isLoggedIn) return; - const auth = await userAuth(); - if (!auth) return; - this.lobbySocket.sendMapVote(auth.jwt, loadStoredMapVotes()); + const jwt = await this.getAuthJwt(); + if (!jwt) return; + this.lobbySocket.sendMapVote(jwt, loadStoredMapVotes()); } private async handleMapVoteChange( event: CustomEvent<{ maps: GameMapType[] }>, ) { - if (!this.isLoggedIn) return; - const auth = await userAuth(); - if (!auth) return; - this.lobbySocket.sendMapVote(auth.jwt, event.detail.maps); + const jwt = await this.getAuthJwt(); + if (!jwt) return; + this.lobbySocket.sendMapVote(jwt, event.detail.maps); }
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@resources/lang/en.json`:
- Around line 334-343: The localization file is missing the key used for the
vote button aria/title (public_lobby.vote_button), so the raw key is exposed to
users and screen readers; add a new entry "public_lobby.vote_button" with a
concise accessible label (e.g., "Vote" or "Vote for maps") into the same JSON
block alongside keys like "vote_submit" and "vote_toast_submitted" so
PublicLobby can use public_lobby.vote_button for aria-label/title instead of
showing the raw key.
In `@src/client/PublicLobby.ts`:
- Around line 49-57: When handleUserMeResponse detects isLoggedIn flips to
false, send an empty/clear vote to the server (using the last known JWT) before
calling lobbySocket.clearMapVote so the server stops counting the previous vote;
locate the same logic in the other user/logout handler (the block referenced at
lines ~96-117) and apply the same change: call the LobbySocket method that sends
a vote with an empty selection and include the stored JWT, await or handle the
promise result (or fire-and-forget consistently with sendStoredVotes), then call
lobbySocket.clearMapVote and update isLoggedIn; ensure you reference
sendStoredVotes, handleUserMeResponse, and lobbySocket.clearMapVote when making
the edits.
In `@src/server/Master.ts`:
- Around line 161-169: The normalizeVotedMaps function currently iterates the
entire incoming maps array which a client can make arbitrarily large; change
normalizeVotedMaps to cap work by stopping once you've collected all possible
public maps (use publicLobbyMapSet.size) or by only iterating the first N
entries (N = publicLobbyMapSet.size) so you never process more than the number
of public maps; update the loop in normalizeVotedMaps to either slice maps to
maps.slice(0, publicLobbyMapSet.size) before iterating or break out once
unique.size === publicLobbyMapSet.size, referencing the existing
normalizeVotedMaps function, its maps parameter, and the publicLobbyMapSet to
locate the change.
- Around line 241-244: The current check rejects tokens when verifyClientToken
returns type "success" but claims is null (used for dev persistent IDs); update
the condition in Master.ts so that a verification of type "success" is accepted
even if claims is null for development/dev persistent IDs—either by gating the
relaxed check behind an environment flag (e.g. NODE_ENV==='development' or
config.allowDevPersistentIds) or by recognizing the dev persistent ID case from
the token; modify the if that uses verification.type and verification.claims
(and the log.warn message) to only return for non-success types or for success
without claims when not in the allowed dev mode.
♻️ Duplicate comments (1)
src/client/MapVoteModal.ts (1)
104-113: Make map tiles keyboard-focusable.
Clickable divs are not focusable; use a button (or add role/tabindex + key handlers) so keyboard users can vote.Proposed fix
- <div - `@click`=${() => this.toggleMapSelection(mapValue)} - class="cursor-pointer transition-transform duration-200 active:scale-95" - > + <button + type="button" + `@click`=${() => this.toggleMapSelection(mapValue)} + class="cursor-pointer transition-transform duration-200 active:scale-95" + aria-pressed=${this.selectedMaps.has(mapValue)} + > <map-display .mapKey=${mapKey} .selected=${this.selectedMaps.has(mapValue)} .translation=${translateText(`map.${mapKey?.toLowerCase()}`)} ></map-display> - </div> + </button>
🧹 Nitpick comments (4)
src/server/Master.ts (2)
83-85: Avoid shipping the debug vote weight.
A fixed 10,000,000,000 weight will overpower base map weights and make a single vote dominate. Please move this to config/env or lower it to a realistic default before merge. If you want, I can help wire a config knob.
114-159: Reduce duplicate broadcast loops.
broadcastLobbies,broadcastMapVoteRequest, andbroadcastMapVoteStatsrepeat the same open/closed cleanup. A small helper would reduce drift.src/client/LobbySocket.ts (1)
87-97: Use a typed union for lobby WS messages.
This keeps parsing strict and makes the handler exhaustive and more idiomatic TypeScript.Proposed refactor
type LobbyUpdateHandler = (lobbies: GameInfo[]) => void; type VoteRequestHandler = () => void; type VoteStatsHandler = (activeVoteCount: number) => void; +type LobbyMessage = + | { type: "lobbies_update"; data: { lobbies: GameInfo[] } } + | { type: "map_vote_request" } + | { type: "map_vote_stats"; data: { activeVoteCount: number } }; @@ private handleMessage(event: MessageEvent) { try { - const message = JSON.parse(event.data as string); - if (message.type === "lobbies_update") { - this.onLobbiesUpdate(message.data?.lobbies ?? []); - } else if (message.type === "map_vote_request") { - this.onVoteRequest?.(); - } else if (message.type === "map_vote_stats") { - const activeVoteCount = Number(message.data?.activeVoteCount ?? 0); - this.onVoteStats?.(activeVoteCount); - } + const message = JSON.parse(event.data as string) as LobbyMessage; + switch (message.type) { + case "lobbies_update": + this.onLobbiesUpdate(message.data?.lobbies ?? []); + break; + case "map_vote_request": + this.onVoteRequest?.(); + break; + case "map_vote_stats": { + const activeVoteCount = Number(message.data?.activeVoteCount ?? 0); + this.onVoteStats?.(activeVoteCount); + break; + } + } } catch (error) {src/client/MapVoteModal.ts (1)
14-16: Prefer composition over inheritance for this modal.
Consider composingBaseModalinside this component rather than extending it to keep behavior flat and easier to test.
Comment on lines
+334
to
+343
| "started": "Started", | ||
| "vote_cta": "Vote for next map!", | ||
| "vote_count_label": "{count, plural, one {# vote influences the next map} other {# votes influence the next map}}", | ||
| "vote_count_tooltip": "Number of players currently voting (connected & waiting in public lobby)", | ||
| "vote_title": "Map Voting", | ||
| "vote_description": "Select any number of maps to influence the next public lobby.", | ||
| "vote_saved": "Your selection is saved on this device.", | ||
| "vote_login_required": "Log in to submit votes.", | ||
| "vote_submit": "Vote", | ||
| "vote_toast_submitted": "Vote submitted!" |
Contributor
There was a problem hiding this comment.
Add the missing vote button label.
PublicLobby uses public_lobby.vote_button for aria-label/title, but it is not defined here, so the raw key will show to users and screen readers.
Proposed fix
"started": "Started",
"vote_cta": "Vote for next map!",
+ "vote_button": "Map voting",
"vote_count_label": "{count, plural, one {# vote influences the next map} other {# votes influence the next map}}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "started": "Started", | |
| "vote_cta": "Vote for next map!", | |
| "vote_count_label": "{count, plural, one {# vote influences the next map} other {# votes influence the next map}}", | |
| "vote_count_tooltip": "Number of players currently voting (connected & waiting in public lobby)", | |
| "vote_title": "Map Voting", | |
| "vote_description": "Select any number of maps to influence the next public lobby.", | |
| "vote_saved": "Your selection is saved on this device.", | |
| "vote_login_required": "Log in to submit votes.", | |
| "vote_submit": "Vote", | |
| "vote_toast_submitted": "Vote submitted!" | |
| "started": "Started", | |
| "vote_cta": "Vote for next map!", | |
| "vote_button": "Map voting", | |
| "vote_count_label": "{count, plural, one {# vote influences the next map} other {# votes influence the next map}}", | |
| "vote_count_tooltip": "Number of players currently voting (connected & waiting in public lobby)", | |
| "vote_title": "Map Voting", | |
| "vote_description": "Select any number of maps to influence the next public lobby.", | |
| "vote_saved": "Your selection is saved on this device.", | |
| "vote_login_required": "Log in to submit votes.", | |
| "vote_submit": "Vote", | |
| "vote_toast_submitted": "Vote submitted!" |
🤖 Prompt for AI Agents
In `@resources/lang/en.json` around lines 334 - 343, The localization file is
missing the key used for the vote button aria/title (public_lobby.vote_button),
so the raw key is exposed to users and screen readers; add a new entry
"public_lobby.vote_button" with a concise accessible label (e.g., "Vote" or
"Vote for maps") into the same JSON block alongside keys like "vote_submit" and
"vote_toast_submitted" so PublicLobby can use public_lobby.vote_button for
aria-label/title instead of showing the raw key.
Comment on lines
+49
to
+57
| private handleUserMeResponse = (event: Event) => { | ||
| const customEvent = event as CustomEvent<UserMeResponse | false>; | ||
| this.isLoggedIn = hasLinkedAccount(customEvent.detail); | ||
| if (this.isLoggedIn) { | ||
| void this.sendStoredVotes(); | ||
| } else { | ||
| this.lobbySocket.clearMapVote(); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Clear the server vote on logout.
When isLoggedIn flips false you only clear the pending client vote; the server still counts the last vote until disconnect. Send an empty vote using the last JWT before clearing to keep counts accurate.
Proposed fix
`@state`() private isLoggedIn: boolean = false;
`@state`() private activeVoteCount: number = 0;
+ private lastVoteToken: string | null = null;
@@
private handleUserMeResponse = (event: Event) => {
const customEvent = event as CustomEvent<UserMeResponse | false>;
this.isLoggedIn = hasLinkedAccount(customEvent.detail);
if (this.isLoggedIn) {
void this.sendStoredVotes();
} else {
+ if (this.lastVoteToken) {
+ this.lobbySocket.sendMapVote(this.lastVoteToken, []);
+ this.lastVoteToken = null;
+ }
this.lobbySocket.clearMapVote();
}
};
@@
private async sendStoredVotes() {
if (!this.isLoggedIn) return;
const auth = await userAuth();
if (!auth) return;
+ this.lastVoteToken = auth.jwt;
this.lobbySocket.sendMapVote(auth.jwt, loadStoredMapVotes());
}
@@
private async handleMapVoteChange(
event: CustomEvent<{ maps: GameMapType[] }>,
) {
if (!this.isLoggedIn) return;
const auth = await userAuth();
if (!auth) return;
+ this.lastVoteToken = auth.jwt;
this.lobbySocket.sendMapVote(auth.jwt, event.detail.maps);
}
@@
private async handleMapVoteSubmit(maps: GameMapType[]) {
if (!this.isLoggedIn) return;
const auth = await userAuth();
if (!auth) return;
+ this.lastVoteToken = auth.jwt;
this.lobbySocket.sendMapVote(auth.jwt, maps);
}Also applies to: 96-117
🤖 Prompt for AI Agents
In `@src/client/PublicLobby.ts` around lines 49 - 57, When handleUserMeResponse
detects isLoggedIn flips to false, send an empty/clear vote to the server (using
the last known JWT) before calling lobbySocket.clearMapVote so the server stops
counting the previous vote; locate the same logic in the other user/logout
handler (the block referenced at lines ~96-117) and apply the same change: call
the LobbySocket method that sends a vote with an empty selection and include the
stored JWT, await or handle the promise result (or fire-and-forget consistently
with sendStoredVotes), then call lobbySocket.clearMapVote and update isLoggedIn;
ensure you reference sendStoredVotes, handleUserMeResponse, and
lobbySocket.clearMapVote when making the edits.
Comment on lines
+161
to
+169
| function normalizeVotedMaps(maps: GameMapType[]): Set<GameMapType> { | ||
| const unique = new Set<GameMapType>(); | ||
| maps.forEach((map) => { | ||
| if (publicLobbyMapSet.has(map)) { | ||
| unique.add(map); | ||
| } | ||
| }); | ||
| return unique; | ||
| } |
Contributor
There was a problem hiding this comment.
Cap map list size before normalizing.
A client can send a very large maps array, and we loop all of it. Limit to the number of public maps to avoid extra CPU/memory work.
Proposed fix
function normalizeVotedMaps(maps: GameMapType[]): Set<GameMapType> {
const unique = new Set<GameMapType>();
- maps.forEach((map) => {
+ const cappedMaps =
+ maps.length > publicLobbyMapSet.size
+ ? maps.slice(0, publicLobbyMapSet.size)
+ : maps;
+ cappedMaps.forEach((map) => {
if (publicLobbyMapSet.has(map)) {
unique.add(map);
}
});
return unique;
}🤖 Prompt for AI Agents
In `@src/server/Master.ts` around lines 161 - 169, The normalizeVotedMaps function
currently iterates the entire incoming maps array which a client can make
arbitrarily large; change normalizeVotedMaps to cap work by stopping once you've
collected all possible public maps (use publicLobbyMapSet.size) or by only
iterating the first N entries (N = publicLobbyMapSet.size) so you never process
more than the number of public maps; update the loop in normalizeVotedMaps to
either slice maps to maps.slice(0, publicLobbyMapSet.size) before iterating or
break out once unique.size === publicLobbyMapSet.size, referencing the existing
normalizeVotedMaps function, its maps parameter, and the publicLobbyMapSet to
locate the change.
Comment on lines
+241
to
+244
| const verification = await verifyClientToken(token, config); | ||
| if (verification.type !== "success" || !verification.claims) { | ||
| log.warn("Rejected map vote from unauthenticated client"); | ||
| return; |
Contributor
There was a problem hiding this comment.
Dev tokens are rejected by the claims check.
verifyClientToken returns claims: null for dev persistent IDs, so votes will not work in dev. If dev should work, allow success without claims (or gate by env).
Proposed fix
const verification = await verifyClientToken(token, config);
- if (verification.type !== "success" || !verification.claims) {
+ if (verification.type !== "success") {
log.warn("Rejected map vote from unauthenticated client");
return;
}
+ if (!verification.claims && config.env() !== GameEnv.Dev) {
+ log.warn("Rejected map vote from unauthenticated client");
+ return;
+ }🤖 Prompt for AI Agents
In `@src/server/Master.ts` around lines 241 - 244, The current check rejects
tokens when verifyClientToken returns type "success" but claims is null (used
for dev persistent IDs); update the condition in Master.ts so that a
verification of type "success" is accepted even if claims is null for
development/dev persistent IDs—either by gating the relaxed check behind an
environment flag (e.g. NODE_ENV==='development' or config.allowDevPersistentIds)
or by recognizing the dev persistent ID case from the token; modify the if that
uses verification.type and verification.claims (and the log.warn message) to
only return for non-success types or for success without claims when not in the
allowed dev mode.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #2999
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
aotumuri