Publiclobbymodal #2975
Publiclobbymodal #2975
Conversation
WalkthroughAdds a public lobby modal and JoinPublicLobbyModal component, a TurnstileManager for join tokens, per-game client IDs, server-side "lobby_info" broadcasts plus socket-replace logic, and integrates these into client join flows, translations, and dev proxy behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Modal as JoinPublicLobbyModal
participant Client as Main (Client)
participant TM as TurnstileManager
participant Server as GameServer
participant UI as Renderer
User->>Modal: Open public lobby UI / click join
Modal->>Client: Dispatch "join-lobby" (source: "public", publicLobbyInfo)
Client->>TM: getTokenForJoin(gameStartInfo?)
TM->>TM: ensureToken (reuse or fetch)
TM-->>Client: Return token or null
Client->>Server: POST join (clientID, token, gameID)
Server->>Server: replaceClientSocket() if reconnect
Server->>Server: startLobbyInfoBroadcast()
loop every 1s
Server->>Client: "lobby_info" (GameInfo)
Client->>Modal: Dispatch DOM "lobby-info"
Modal->>UI: updateFromLobby() -> render players, countdown, status
end
Server->>Client: "start" when game begins
Server->>Server: stopLobbyInfoBroadcast()
Modal->>Modal: Cleanup timers / close or transition
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
🤖 Fix all issues with AI agents
In `@src/client/JoinPublicLobbyModal.ts`:
- Around line 302-311: The lobby callback in startLobbySocket currently only
handles non-null lobbies via updateFromLobby, so when WorkerLobbySocket emits
null on lobby_closed the UI remains stale; update startLobbySocket's callback to
explicitly handle lobby === null by closing the JoinPublicLobbyModal (call the
modal close/dismiss method used in this class) and trigger a redirect/navigation
to the appropriate page (or emit the same event path used elsewhere), otherwise
call updateFromLobby(lobby) as before; reference startLobbySocket,
this.lobbySocket, WorkerLobbySocket, and updateFromLobby to locate where to add
the null branch.
- Around line 24-36: The countdown freezes because render() uses Date.now() but
no timer triggers re-renders; add a 1-second setInterval when lobbyStartAt is
non-null that calls this.requestUpdate() to force updates while the lobby start
time is set (store the interval id on the instance, e.g.
this._countdownTimerId), ensure you only create one timer and clear it when
lobbyStartAt becomes null and also clear it in onClose() using
clearInterval(this._countdownTimerId) to avoid leaks; reference render(),
lobbyStartAt, requestUpdate(), and onClose() when making these changes.
- Around line 43-47: The onBack handler is passed as this.closeAndLeave which
loses the class instance context; update the modalHeader call to wrap the method
so it preserves this (e.g., use an arrow wrapper or bind) — replace onBack:
this.closeAndLeave with a bound wrapper such as onBack: () =>
this.closeAndLeave() (or onBack: this.closeAndLeave.bind(this)) so
closeAndLeave() can safely call this.close() and other instance methods.
🧹 Nitpick comments (4)
src/client/LobbySocket.ts (2)
215-240: Consider handling config fetch failure more gracefully.If
getServerConfigFromClient()throws,handleConnectErroris called, which increments attempts and may schedule reconnect. However, a persistent config fetch failure (e.g., network down) will silently exhaust retries without user feedback.This is acceptable for now since the feature is new, but consider adding an
onErrorcallback option for surfacing connection failures to the UI.
274-301: No fallback mechanism after max attempts.Unlike
PublicLobbySocketwhich falls back to HTTP polling,WorkerLobbySocketsilently stops aftermaxWsAttempts. The user will see stale data with no indication of failure.Consider either:
- Adding an HTTP fallback endpoint
- Passing an
onMaxAttemptsReachedcallback to notify the UIsrc/client/Main.ts (1)
206-216: Use a discriminated union forJoinLobbyEvent.
sourceandpublicLobbyInfocan drift. A union makespublicLobbyInforequired only whensource: "public"and keeps other cases clean.♻️ Suggested typing
-export interface JoinLobbyEvent { - clientID: string; - // Multiplayer games only have gameID, gameConfig is not known until game starts. - gameID: string; - // GameConfig only exists when playing a singleplayer game. - gameStartInfo?: GameStartInfo; - // GameRecord exists when replaying an archived game. - gameRecord?: GameRecord; - source?: "public" | "private" | "host" | "matchmaking" | "singleplayer"; - publicLobbyInfo?: GameInfo; -} +type JoinLobbyEventBase = { + clientID: string; + gameID: string; + gameStartInfo?: GameStartInfo; + gameRecord?: GameRecord; +}; + +export type JoinLobbyEvent = + | (JoinLobbyEventBase & { + source: "public"; + publicLobbyInfo: GameInfo; + }) + | (JoinLobbyEventBase & { + source?: "private" | "host" | "matchmaking" | "singleplayer"; + publicLobbyInfo?: undefined; + });src/client/JoinPublicLobbyModal.ts (1)
12-13: Prefer composition over inheritance for the modal.Extending
BaseModalcouples lifecycle and UI. A composed modal wrapper keeps this component focused on state/render and is easier to test.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/LobbySocket.ts`:
- Around line 225-228: The WebSocket URL construction in LobbySocket.ts can
produce a double-slash if config.workerPath(this.lobbyId) returns
leading/trailing slashes; update the wsUrl build to normalize the worker path
before concatenation by trimming any leading/trailing slashes (use the value
returned by config.workerPath(this.lobbyId)), then join with a single slash when
constructing wsUrl (referencing protocol, window.location.host,
config.workerPath, this.lobbyId, and wsUrl) so the final URL never contains
duplicated slashes.
- Around line 276-304: The reconnect logic in handleClose and handleConnectError
currently just logs when wsConnectionAttempts >= maxWsAttempts and doesn't
notify the rest of the system; add a single terminal notification and stop
further retries: when wsConnectionAttempts >= this.maxWsAttempts in both
handleClose and handleConnectError, call a single new/explicit notification
method (e.g. this.onTerminalFailure() or emit a "terminalFailure" event) and
ensure it's only invoked once (reuse this.wsAttemptCounted or add a dedicated
flag) instead of scheduling reconnects via scheduleReconnect; update
callers/consumers to handle that notification.
🧹 Nitpick comments (1)
src/client/JoinPublicLobbyModal.ts (1)
199-237: UseGameModeconstant instead of string literal.
This keeps comparisons typed and avoids drift if the literal ever changes. Please verifyGameMode.FFAmatches the actual value.Suggested update
- const modeName = - c.gameMode === "Free For All" + const modeName = + c.gameMode === GameMode.FFA ? translateText("game_mode.ffa") : translateText("game_mode.teams"); @@ - ${c.gameMode !== "Free For All" && c.playerTeams + ${c.gameMode !== GameMode.FFA && c.playerTeams ? this.renderConfigItem(
ryanbarlow97
added
UI/UX
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/JoinPublicLobbyModal.ts`:
- Around line 335-375: In pollLobby, the code mistakenly subtracts Date.now()
from the server-provided duration msUntilStart; update pollLobby (in the private
async pollLobby(lobbyId: string) function) to pass msUntilStart through
unchanged (or keep it undefined if absent) when calling this.updateFromLobby so
it remains a duration (matching updateFromLobby's handling), i.e. remove the
"Math.max(0, msUntilStart - Date.now())" adjustment and send msUntilStart
directly.
🧹 Nitpick comments (2)
src/client/JoinPublicLobbyModal.ts (2)
12-25: Prefer composition over extendingBaseModalfor new UI components.Using inheritance couples this element to
BaseModalbehavior. If you can, wrap<o-modal>and delegate instead of extending, and consider a small typed union for lobby status to reducenullchecks.
194-240: Replace string literals withGameMode.FFAenum value.The code currently compares
gameModeagainst the string"Free For All"in two places. SinceGameModeis already imported andGameMode.FFA = "Free For All", use the enum constant instead to avoid drift if the enum value changes.🔧 Suggested changes
const modeName = - c.gameMode === "Free For All" + c.gameMode === GameMode.FFA ? translateText("game_mode.ffa") : translateText("game_mode.teams"); ... - ${c.gameMode !== "Free For All" && c.playerTeams + ${c.gameMode !== GameMode.FFA && c.playerTeams ? this.renderConfigItem(
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/JoinPublicLobbyModal.ts`:
- Around line 151-172: The element can be removed from the DOM without calling
close(), leaking countdownTimerId and lobbyPollInterval; implement a
disconnectedCallback on the JoinPublicLobbyModal class that performs the same
cleanup as onClose (or simply call this.onClose()) to call
clearCountdownTimer(), stopLobbyPolling(), and handle leaving the lobby if
leaveLobbyOnClose is set, ensuring timers are stopped when the element is
detached.
🧹 Nitpick comments (1)
src/client/JoinPublicLobbyModal.ts (1)
251-265: Consider extractingunitKeysto a shared constant.This mapping duplicates knowledge about unit names and their translation keys. If unit names change or new units are added, this could drift out of sync with other parts of the codebase.
The fallback to the raw
unitname (line 277) is a good safety net. For now this works, but you might want to extract this mapping to a shared file (e.g.,UnitTranslations.ts) if other components need the same lookup.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/JoinPublicLobbyModal.ts`:
- Around line 211-215: The mode label only inspects c.gameMode (compared to
GameMode.FFA) when computing modeName, so HumansVsNations lobbies (which are
represented via c.playerTeams) are incorrectly labeled as "teams"; update the
logic that sets modeName to also check c.playerTeams for the HumansVsNations
value and return the appropriate translation key (e.g.,
translateText("game_mode.humans_vs_nations")) when matched, otherwise fall back
to the existing GameMode.FFA vs teams branch; look for the modeName assignment
and uses of c.gameMode, GameMode.FFA, translateText(...) and c.playerTeams to
implement the conditional.
♻️ Duplicate comments (1)
src/client/JoinPublicLobbyModal.ts (1)
150-166: AdddisconnectedCallbackto stop timers when the element is detached.
onClose()clears timers, but if the element is removed from the DOM withoutclose(), the countdown timer and polling interval will keep running. Add adisconnectedCallback()cleanup so timers always stop.🔧 Suggested fix
+ disconnectedCallback() { + super.disconnectedCallback(); + this.clearCountdownTimer(); + this.stopLobbyPolling(); + } + protected onClose(): void { this.clearCountdownTimer(); this.stopLobbyPolling();
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/Main.ts`:
- Around line 625-631: The onHashUpdate handler currently closes modals and
calls handleLeaveLobby but doesn't cancel any in-flight join/prefetch, allowing
a previous join Promise to complete after navigation; add an explicit
cancellation mechanism (e.g., an AbortController or incremental joinId token
stored on the instance like this.joinAbortController or this.currentJoinId) that
is set/created when starting the join/prefetch and checked/cancelled in the join
completion path, then update onHashUpdate to call abort() or increment/clear the
token before closing this.joinModal/this.joinPublicModal and invoking
handleLeaveLobby so any ongoing join is invalidated and cannot complete. Ensure
join logic (where the Promise resolves to actually join) respects the
abort/token (checking controller.signal.aborted or matching currentJoinId) and
returns early when cancelled.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/Main.ts`:
- Around line 626-633: onHashUpdate can be triggered by a hashchange fired after
a popstate rollback and close the lobby unexpectedly; add a one-shot guard flag
(e.g. skipNextHashChange) that you set right before you perform the
popstate/rollback action and check at the top of onHashUpdate to return early
and reset the flag; update the rollback code path to set skipNextHashChange =
true before calling history.back()/popstate emulation and ensure onHashUpdate
checks skipNextHashChange first (and clears it) before calling
cancelJoinInFlight(), this.joinModal?.close(), this.joinPublicModal?.close(), or
this.handleLeaveLobby() so the rollback cannot immediately re-close the lobby.
Description:
adds a join public lobby modal
also fixes some issues with clicking/unclicking the button (turnstile rework)
also made a "connecting" to the lobby
also allows people to rejoin games as we now store a clientID per gameID locally
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n