Delayed lobby start#4184
Conversation
Fixes openfrontio#4169 Adds a delayed lobby start option.
|
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:
WalkthroughHost can set a start-delay (seconds). Clients and server use a new toggle event to schedule or cancel a future startsAt; schemas and translations updated. Host and join modals show a server-synced countdown/status. ChangesDelayed Private Lobby Start
Sequence DiagramsequenceDiagram
participant Host as HostLobbyModal
participant Main as Main/EventBus
participant EventBus as EventBus
participant Transport as Transport
participant Server as GameServer
Host->>Main: dispatch "toggle_game_start_timer"
Main->>EventBus: emit SendToggleGameStartTimer()
EventBus->>Transport: SendToggleGameStartTimer()
Transport->>Server: send intent "toggle_game_start_timer"
Server->>Server: clear startsAt OR set startsAt = now + (startDelay ?? 0)*1000
Note over Server,Host: Clients read startsAt and serverTime to show synchronized countdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/HostLobbyModal.ts (1)
559-617:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset new state fields in onClose cleanup.
lobbyStartAtandserverTimeOffset(introduced at lines 97–98) are not reset in theonClosecleanup block. Although stale values are unlikely to cause issues (lobby updates stop on close), resetting them ensures a clean slate for consistency with the comment at line 578.Proposed fix
this.lobbyId = ""; this.clients = []; this.lobbyCreatorClientID = ""; + this.lobbyStartAt = null; + this.serverTimeOffset = 0; this.goldMultiplier = false;🤖 Prompt for 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. In `@src/client/HostLobbyModal.ts` around lines 559 - 617, The onClose cleanup in HostLobbyModal does not reset the recently added fields lobbyStartAt and serverTimeOffset; update the onClose method to explicitly clear those two properties (lobbyStartAt and serverTimeOffset) alongside the other transient state resets so the HostLobbyModal instance returns to a clean slate after closing.
🧹 Nitpick comments (2)
src/client/Main.ts (1)
1011-1016: ⚡ Quick winConsider typing the
start-gameevent detail.The event detail is accessed at line 1012 but the interface at line 222 declares
"start-game": CustomEventwithout a detail type. AddingCustomEvent<{ startDelay: number }>would catch shape mismatches at compile time.♻️ Suggested type improvement
At line 222:
"join-lobby": CustomEvent<JoinLobbyEvent>; "kick-player": CustomEvent; - "start-game": CustomEvent; + "start-game": CustomEvent<{ startDelay: number }>; "join-changed": CustomEvent<undefined>;🤖 Prompt for 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. In `@src/client/Main.ts` around lines 1011 - 1016, The "start-game" CustomEvent is untyped so accesses to event.detail aren't checked; update the event map entry that currently declares "start-game": CustomEvent to "start-game": CustomEvent<{ startDelay: number }>, then change the handler signature for handleStartGame to accept CustomEvent<{ startDelay: number }> (or narrow the parameter) so event.detail.startDelay is type-checked; ensure SendStartGameEvent usage remains the same in the body (this.eventBus.emit(new SendStartGameEvent(startDelay))).src/client/JoinLobbyModal.ts (1)
175-175: 💤 Low valueConsider using a consistent translation key for both lobby types.
The unified status panel uses
public_lobby.statusfor both public and private lobbies. While the text "Status" is likely identical, using a public-specific key in private lobby contexts is semantically inconsistent. Consider usingcommon.statusor conditionally selecting the key based onisPrivateLobby().♻️ Suggested fix using a shared translation key
- >${translateText("public_lobby.status")}</span + >${translateText("common.status")}</spanAlternatively, if
common.statusdoesn't exist, conditionally pick the key:- >${translateText("public_lobby.status")}</span + >${translateText(this.isPrivateLobby() ? "private_lobby.status" : "public_lobby.status")}</span🤖 Prompt for 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. In `@src/client/JoinLobbyModal.ts` at line 175, The translation key used in the status label inside JoinLobbyModal currently hardcodes "public_lobby.status"; update the translateText call to use a shared key (e.g., "common.status") or choose the key conditionally based on isPrivateLobby() so the label is semantically correct for both lobby types — locate the translateText invocation in JoinLobbyModal (where the JSX renders >${translateText("public_lobby.status")}) and replace it with either translateText("common.status") or translateText(isPrivateLobby() ? "private_lobby.status" : "public_lobby.status").
🤖 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 `@src/client/HostLobbyModal.ts`:
- Around line 868-882: The startDelay input handler handleStartDelayValueChanges
uses parseBoundedFloatFromInput with bounds {min: 0.1, max: 1000} that mismatch
the toggle-input-card UI (1–600s); update the parser call to enforce the UI
bounds by using {min: 1, max: 600} and, if fractional seconds aren’t required,
swap to parseBoundedIntegerFromInput so startDelayValue, input.value handling,
and the subsequent putGameConfig() receive only valid integer seconds.
- Around line 1105-1107: When building the event payload in HostLobbyModal where
you set detail: { startDelay: this.startDelayValue }, first check the startDelay
toggle state and the parsed numeric value; if the toggle is OFF or the value is
missing/invalid, send 0 instead of this.startDelayValue. Update the payload
creation (the block that constructs detail.startDelay) to use the toggle state
(e.g., startDelayToggle / whatever toggle prop is used) and a safe
parse/coalesce of this.startDelayValue to 0 so you never send undefined or a
stale value.
- Around line 80-81: The default delay values are inconsistent: the component
state initializes startDelayValue = 3 while the toggle-input-card uses
.defaultInputValue=${5}; update the toggle-input-card's .defaultInputValue to 3
to match the component state and PR description (or alternatively set
startDelayValue to 5 if you prefer the 5s behavior) so startDelayValue and the
toggle-input-card's defaultInputValue are identical; locate the
toggle-input-card usage and adjust its .defaultInputValue accordingly
(references: startDelayValue, startDelay, and the toggle-input-card element).
In `@src/core/Schemas.ts`:
- Line 486: The startDelay field in the schema currently allows any number;
update the validation for startDelay in the schema definition (the startDelay
property in src/core/Schemas.ts) to enforce UI-aligned bounds by replacing
z.number() with a bounded validator such as z.number().min(1).max(600) (or
.max(3600) if you prefer a 1-hour cap) so negative or extremely large values are
rejected and adhere to the UI constraints.
In `@src/server/GameServer.ts`:
- Around line 517-518: The code sets the game start time using
stampedIntent.startDelay without bounds checking, allowing negative or huge
delays; validate stampedIntent.startDelay (or add constraints to
StartGameIntentSchema) to enforce a reasonable range (e.g., 0–60 seconds) before
calling this.setStartsAt(Date.now() + stampedIntent.startDelay * 1000); if out
of range, clamp to the bounds or reject/replace with a default (0 or 60) and
log/handle the invalid intent so GameServer.start logic (the setStartsAt call)
never receives an unsafe value.
In `@src/server/WorkerLobbyService.ts`:
- Line 88: The WorkerLobbyService is incorrectly offsetting the incoming
absolute epoch-ms startsAt; remove the +1000000 so the lobby uses the exact
timestamp sent by the master: update the call in WorkerLobbyService from
game.setStartsAt(msg.startsAt + 1000000) to use game.setStartsAt(msg.startsAt),
ensuring msg.startsAt (as produced by MasterLobbyService) is treated as an
absolute milliseconds timestamp compatible with GameServer's checks (Date.now()
< this.startsAt etc.).
---
Outside diff comments:
In `@src/client/HostLobbyModal.ts`:
- Around line 559-617: The onClose cleanup in HostLobbyModal does not reset the
recently added fields lobbyStartAt and serverTimeOffset; update the onClose
method to explicitly clear those two properties (lobbyStartAt and
serverTimeOffset) alongside the other transient state resets so the
HostLobbyModal instance returns to a clean slate after closing.
---
Nitpick comments:
In `@src/client/JoinLobbyModal.ts`:
- Line 175: The translation key used in the status label inside JoinLobbyModal
currently hardcodes "public_lobby.status"; update the translateText call to use
a shared key (e.g., "common.status") or choose the key conditionally based on
isPrivateLobby() so the label is semantically correct for both lobby types —
locate the translateText invocation in JoinLobbyModal (where the JSX renders
>${translateText("public_lobby.status")}) and replace it with either
translateText("common.status") or translateText(isPrivateLobby() ?
"private_lobby.status" : "public_lobby.status").
In `@src/client/Main.ts`:
- Around line 1011-1016: The "start-game" CustomEvent is untyped so accesses to
event.detail aren't checked; update the event map entry that currently declares
"start-game": CustomEvent to "start-game": CustomEvent<{ startDelay: number }>,
then change the handler signature for handleStartGame to accept CustomEvent<{
startDelay: number }> (or narrow the parameter) so event.detail.startDelay is
type-checked; ensure SendStartGameEvent usage remains the same in the body
(this.eventBus.emit(new SendStartGameEvent(startDelay))).
🪄 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: 8d6b26b8-f32d-49cd-b844-f859edde3578
📒 Files selected for processing (8)
resources/lang/en.jsonsrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/Main.tssrc/client/Transport.tssrc/core/Schemas.tssrc/server/GameServer.tssrc/server/WorkerLobbyService.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Im not sure what you want fixed about that line? |
|
If you are talking about the code rabbit thing, that is fixed on the newest commit. But code rabbit didnt review again for some. |
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Looks good, but I think we should have a way for the host to cancel the start.
Instead I think we should create a GameStartTimer event that triggers when the game will start, and it also let's us remove theh game start time. Once the game start has been triggered, clicking the "start game" button will cancel it.
|
Im not 100% sure what you mean by that. We currently have one event for starting a game. "start-game", that i added the start-time to. Do you want me to replace this event with this "game-start-timer" event? Or do you want me to add it as an additional event. Should we do the same for public lobies? And then how should the event be canceled again? By sending the same event with a time of zero? Or do you have something else in mind? I might try implementing it, but im not 100% sure you are talking about the same event setup that i am. |
something like that, actually we could add one more event like "cancel-game-start" or something would work. |
…io#4185 and to make it more similar to other settings
An example of how it looks now from the perspective of the host.
Also there is one event that replaces the old GameStart event called ToggleGameStartTimer, and i added the actual start delay to the config of the lobby. This both prepares for #4185 and is more consistent with the other settings. @coderabbitai can you review again? |
|
The Comment image shows the updated "CREATE PRIVATE LOBBY" modal. It includes all the existing game config toggles (INSTANT BUILD, RANDOM SPAWN, etc.) plus a new START DELAY (SECONDS) card — highlighted in blue with a checked checkbox and a value of 90 entered. At the bottom, a full-width blue button reads "STARTING IN 1MIN 25S. CLICK TO CANCEL", showing the countdown with cancel support. The layout fits cleanly alongside the other numeric config options (GAME LENGTH, PVP IMMUNITY DURATION, GOLD MULTIPLIER, STARTING GOLD). The UI looks clean and well integrated. The Re-reviewing after the latest commits that introduce ✅ Action performedFull review finished. |
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 723: The placeholder "start_delay_placeholder" in resources/lang/en.json
is set to "60" but the control's actual default delay is 3; update the
"start_delay_placeholder" value from "60" to "3" so the UI placeholder matches
the control's default behavior and avoids confusion when configuring private
lobbies.
In `@src/client/HostLobbyModal.ts`:
- Around line 80-81: In HostLobbyModal, reset the newly added state fields
inside onClose(): set startDelay back to its initial true and startDelayValue
back to 3 (or undefined if you prefer the original default behavior) so stale
timer/delay values don't leak into subsequent lobby sessions; locate the
onClose() method and add assignments for startDelay and startDelayValue
alongside the other reset logic.
- Around line 468-470: The start/cancel button is disabled whenever
clients.length < 2 which prevents the host from cancelling an active countdown;
update the disabled binding to also check the timer state so it stays enabled
during an active start timer (e.g. change ?disable=${this.clients.length < 2} to
?disabled=${this.clients.length < 2 && !this.isStartTimerActive}). Ensure the
boolean (isStartTimerActive or equivalent) is set/cleared in the
toggleGameStartTimer handler (or wherever the countdown starts/stops) so the UI
reflects the active countdown and the host can cancel.
🪄 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: 11932860-795a-4549-8ef1-829b4a9ddfd7
📒 Files selected for processing (7)
resources/lang/en.jsonsrc/client/HostLobbyModal.tssrc/client/JoinLobbyModal.tssrc/client/Main.tssrc/client/Transport.tssrc/core/Schemas.tssrc/server/GameServer.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/HostLobbyModal.ts (1)
469-469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winButton remains disabled during active countdown, preventing cancel.
The expression
secondsRemaining ?? this.clients.length < 2evaluates tosecondsRemainingwhen it's not null. When countdown is active (e.g.,secondsRemaining = 5), this passes5to?disable, which is truthy, disabling the button. Host cannot cancel.The fix should check whether a countdown is active explicitly:
Proposed fix
- ?disable=${secondsRemaining ?? this.clients.length < 2} + ?disable=${secondsRemaining === null && this.clients.length < 2}This enables the button when:
- Countdown is active (
secondsRemaining !== null) → can cancel- Countdown is inactive with 2+ players → can start
🤖 Prompt for 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. In `@src/client/HostLobbyModal.ts` at line 469, The button binding currently uses the expression secondsRemaining ?? this.clients.length < 2 which treats a non-null countdown value as truthy and keeps the button disabled; change the template binding in HostLobbyModal (the ?disable/?disabled attribute on the Start/Cancel button) to explicitly check countdown state so the button is disabled only when there is no active countdown AND there are fewer than 2 players (i.e., use a boolean expression like "secondsRemaining === null && this.clients.length < 2" for the attribute), referencing the secondsRemaining property and this.clients array in the template.
🤖 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.
Duplicate comments:
In `@src/client/HostLobbyModal.ts`:
- Line 469: The button binding currently uses the expression secondsRemaining ??
this.clients.length < 2 which treats a non-null countdown value as truthy and
keeps the button disabled; change the template binding in HostLobbyModal (the
?disable/?disabled attribute on the Start/Cancel button) to explicitly check
countdown state so the button is disabled only when there is no active countdown
AND there are fewer than 2 players (i.e., use a boolean expression like
"secondsRemaining === null && this.clients.length < 2" for the attribute),
referencing the secondsRemaining property and this.clients array in the
template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4ae53cb-edc5-420a-b6df-23c447f1cf00
📒 Files selected for processing (2)
resources/lang/en.jsonsrc/client/HostLobbyModal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
2 participants
Resolves #4169
Description:
Adds a delayed lobby start option.
Utilizes the same system as for public lobbies.
The default for the option is for lobbies to take 3 seconds to start, however this can easily be changed.
The current setting is controlled through an enable-disable slider, however there are multiple other options for how to control this.
For example we could do a slider, an input field, a dropdown etc. And i dont necessarily know if the currently implemented option is the best.
Furhtermore im not sure if i have used the language file completely correctly. There is now a duplicate field for both private and public lobby. However there is not category shared between the two. So i decided to reuse the field from public for private games, as this simplified the code a bit.
Host video
https://github.com/user-attachments/assets/6f3db6e4-7323-4fad-8544-efb8cef4d969
Non-host video
https://github.com/user-attachments/assets/ee02a072-1f42-4dde-a5d9-120fda862eb7
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FrederikJA