Skip to content

Conversation

@ryanbarlow97
Copy link
Contributor

@ryanbarlow97 ryanbarlow97 commented Nov 4, 2025

Description:

Finishes & closes #1969 and closes #1806.

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

w.o.n

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

Walkthrough

Adds a lobby creation timestamp to GameStartInfo and ServerStartGameMessage, threads that timestamp into record creation and archiving, computes lobby fill time in createPartialGameRecord, and exposes a new Stats hook lobbyFillTime.

Changes

Cohort / File(s) Summary
Schemas
src/core/Schemas.ts
Added lobbyCreatedAt: number to GameStartInfo and ServerStartGameMessage; added lobbyFillTime: number().nonnegative() to GameEndInfo.
Client join / event types
src/client/SinglePlayerModal.ts, src/client/Main.ts
JoinLobbyEvent payload now includes gameStartInfo.config.lobbyCreatedAt (set with Date.now() in single-player flow); public type updated.
Server start / start messages
src/server/GameServer.ts, src/client/LocalServer.ts
GameStartInfo and start messages now include lobbyCreatedAt (server uses this.createdAt); LocalServer.start() forwards lobbyCreatedAt.
Record creation & utilities
src/core/Util.ts, src/client/ClientGameRunner.ts, src/server/GameServer.ts
createPartialGameRecord signature accepts optional lobbyCreatedAt; computes actualLobbyCreatedAt and lobbyFillTime, and includes both in returned PartialGameRecord.info. Call sites updated to pass lobbyCreatedAt.
Stats API
src/core/game/Stats.ts, src/core/game/StatsImpl.ts
Added lobbyFillTime(fillTimeMs: number): void to Stats interface and StatsImpl (no-op implementation).
Archival payload handling
src/client/LocalServer.ts
End-game archival POST now sends a Blob from compressedData.buffer with MIME application/json (archive payload construction updated).

Sequence Diagram(s)

sequenceDiagram
    participant SP as SinglePlayer (Client)
    participant Client as ClientRuntime
    participant Server as GameServer
    participant Archive as ArchiveService

    rect rgb(248,250,255)
    Note over SP,Server: Lobby creation time set
    SP->>Client: emit JoinLobby (gameStartInfo.config.lobbyCreatedAt = Date.now())
    Server->>Server: GameServer.createdAt set => lobbyCreatedAt
    end

    rect rgb(240,255,245)
    Note over Server,Client: Start propagation
    Server->>Client: ServerStartGameMessage (includes lobbyCreatedAt)
    Client->>Client: LocalServer stores lobbyCreatedAt with game state
    end

    rect rgb(255,250,240)
    Note over Client,Archive: End & archive
    Client->>Client: ClientGameRunner.saveGame(..., lobbyCreatedAt)
    Client->>Client: createPartialGameRecord(actualLobbyCreatedAt, compute lobbyFillTime)
    Client->>Archive: POST Blob(payload) containing record with lobbyCreatedAt & lobbyFillTime
    end

    rect rgb(245,245,245)
    Note over Client: Stats emission
    Client->>Client: Stats.lobbyFillTime(lobbyFillTime)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Attention areas:
    • Fallback logic and correctness in createPartialGameRecord when lobbyCreatedAt is absent.
    • Units and non-negativity of lobbyFillTime computation.
    • Consistency of timestamps between client Date.now() and server this.createdAt.
    • Archive POST change to Blob and MIME type correctness.

Possibly related PRs

Suggested labels

Feature - Frontend

Suggested reviewers

  • scottanderson
  • evanpelle

Poem

A lobby birthed with a timestamp small,
We note the time that summoned all.
When players fill and games begin,
A quiet number traces when —
Stats sing the gap, a tidy call. 🎮

Pre-merge checks

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'lobby fill time added to stats' directly relates to the main objective of adding lobby fill time statistics and closes issue #1806.
Description check ✅ Passed The description references issue #1969 and #1806, confirms completion of required deliverables (UI screenshots, text translation, tests, thorough testing), and provides contact information.
Linked Issues check ✅ Passed The PR implements the core requirement from #1969 (adding lobbyCreatedAt tracking to GameStartInfo schema) and fulfills #1806 (computing lobby fill time in stats) with schema additions, Util logic, and Stats interface methods.
Out of Scope Changes check ✅ Passed All changes are scoped to lobby fill time implementation: schema additions for lobbyCreatedAt and lobbyFillTime, Util logic for computation, Stats interface/implementation for tracking, and client-side propagation of timestamps.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1d96e75 and e562a91.

📒 Files selected for processing (1)
  • src/client/LocalServer.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.

Applied to files:

  • src/client/LocalServer.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.

Applied to files:

  • src/client/LocalServer.ts
🔇 Additional comments (1)
src/client/LocalServer.ts (1)

87-87: ****

The concern about lobbyCreatedAt being undefined is not valid. The code is already protected by multiple safeguards:

  1. Type safety: GameStartInfo defines lobbyCreatedAt as a required field (z.number() in the schema, not optional).
  2. Runtime check: Lines 80–82 throw an error if gameStartInfo is undefined, ensuring the value exists before line 87.
  3. Schema validation: GameStartInfo is created via GameStartInfoSchema.safeParse() in GameServer.ts, which enforces all required fields including lobbyCreatedAt.

After the check at line 80, TypeScript's control flow narrows the type so gameStartInfo cannot be undefined at line 87. Since GameStartInfo requires lobbyCreatedAt, accessing it safely is guaranteed. The suggested validation is redundant.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ryanbarlow97 ryanbarlow97 marked this pull request as ready for review November 4, 2025 23:27
@ryanbarlow97 ryanbarlow97 requested a review from a team as a code owner November 4, 2025 23:27
coderabbitai[bot]
coderabbitai bot previously approved these changes Nov 4, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/core/Util.ts (2)

197-198: Consider clarifying the comment.

The comment states the default is "for singleplayer," but the fallback to start actually applies whenever lobbyCreatedAt is not provided, regardless of game mode. This could happen in multiplayer too if the data is missing.

Consider this wording:

-  // lobby creation time (ms). Defaults to start time for singleplayer.
+  // lobby creation time (ms). Defaults to start time if not provided.
   lobbyCreatedAt?: number,

206-211: The fill time calculation can be simplified.

The nested Math.min is redundant. The outer Math.max(0, ...) already handles the case where actualLobbyCreatedAt > start (clock skew) by clamping to zero.

Consider simplifying:

   // Use start time as lobby creation time for singleplayer
   const actualLobbyCreatedAt = lobbyCreatedAt ?? start;
-  const lobbyFillTime = Math.max(
-    0,
-    start - Math.min(actualLobbyCreatedAt, start),
-  );
+  const lobbyFillTime = Math.max(0, start - actualLobbyCreatedAt);

Both versions produce the same result, but this is more direct.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cbd1389 and 2842bd4.

📒 Files selected for processing (1)
  • src/core/Util.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/Util.ts (4)
src/core/Schemas.ts (1)
  • PartialGameRecord (597-597)
src/server/GameServer.ts (2)
  • end (473-523)
  • start (389-427)
src/client/LocalServer.ts (1)
  • start (53-89)
src/core/game/StatsImpl.ts (1)
  • lobbyFillTime (268-268)
🔇 Additional comments (1)
src/core/Util.ts (1)

213-229: LGTM!

The new fields are correctly integrated into the returned record. The lobbyCreatedAt and lobbyFillTime values are properly computed and included in the info object.

@evanpelle evanpelle added this to the v27 milestone Nov 6, 2025
Copy link
Collaborator

@evanpelle evanpelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@evanpelle evanpelle merged commit 34251c0 into openfrontio:main Nov 11, 2025
8 checks passed
Lavodan pushed a commit to Lavodan/OpenFrontIO_changes that referenced this pull request Nov 11, 2025
Finishes & closes openfrontio#1969 and closes openfrontio#1806.

- [x] I have added screenshots for all UI updates
- [x] I process any text displayed to the user through translateText()
and I've added it to the en.json file
- [x] I have added relevant tests to the test directory
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced

regression is found:

w.o.n

---------

Co-authored-by: Evan <evanpelle@gmail.com>

temp disable factory upgrades
@ryanbarlow97 ryanbarlow97 deleted the lobbytime branch November 11, 2025 20:25
YonderLolasaurus pushed a commit to YonderLolasaurus/OpenFrontIO that referenced this pull request Nov 26, 2025
## Description:

Finishes & closes openfrontio#1969 and closes openfrontio#1806.

## Please complete the following:

- [x] I have added screenshots for all UI updates
- [x] I process any text displayed to the user through translateText()
and I've added it to the en.json file
- [x] I have added relevant tests to the test directory
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced

## Please put your Discord username so you can be contacted if a bug or
regression is found:

w.o.n

---------

Co-authored-by: Evan <evanpelle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Track the time to lobby fill in stats

2 participants