Conversation
|
|
WalkthroughAdds a map-voting feature: UI modal, client storage and WebSocket vote plumbing, lobby UI integration, server-side vote collection/broadcast, and weighted dynamic map selection used by the playlist generator. 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 and selections
User->>Client: Toggle map selections
Client->>Storage: saveStoredMapVotes(maps)
Client-->>User: Update UI
alt User logged in
User->>Client: Submit vote
Client->>WS: Send `map_vote` (token + maps)
WS->>Server: Receive & verify vote
Server->>Server: Update per-user votes & connections
Server->>Server: Aggregate weights
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 a public game
Server->>Playlist: gameConfig(mapVoteWeights)
Playlist->>Playlist: pickWeightedMap(weights + recency)
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}
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
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/server/Master.ts`:
- Around line 83-84: Replace the hardcoded debug constant MAP_VOTE_WEIGHT with a
configurable value sourced from configuration or an environment variable
(suggest VOTE_WEIGHT_MULTIPLIER) and update any code using MAP_VOTE_WEIGHT to
read from that config value instead; also update the MapVoteMessageSchema
definition to add an explicit array size limit by changing the maps validator to
include .max(5) (i.e., maps: z.array(z.nativeEnum(GameMapType)).max(5)) so the
maximum number of maps is enforced in the schema.
🧹 Nitpick comments (2)
src/client/PublicLobby.ts (2)
119-124: Add null check feedback for missing modal.The
querySelectormay return null if the modal isn't mounted. Consider logging a warning to help debug integration issues.Proposed fix
private openMapVoteModal() { const modal = document.querySelector("map-vote-modal") as | (HTMLElement & { open: () => void }) | null; - modal?.open(); + if (modal) { + modal.open(); + } else { + console.warn("map-vote-modal not found in DOM"); + } }
96-117: Auth calls could be deduplicated.
sendStoredVotes,handleMapVoteChange, andhandleMapVoteSubmitall follow the same pattern: check login → get auth → send vote. Consider extracting a helper:Proposed helper
private async withAuth( action: (jwt: string) => void, ): Promise<void> { if (!this.isLoggedIn) return; const auth = await userAuth(); if (!auth) return; action(auth.jwt); }Then use:
private async sendStoredVotes() { await this.withAuth((jwt) => { this.lobbySocket.sendMapVote(jwt, loadStoredMapVotes()); }); }
2 participants
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