Conversation
Contributor
|
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:
WalkthroughServer-config-driven lobby timing, per-map aspect-ratio tracking, responsive lobby card layout and rendering changes; modal stacking/context and multiple UI style tweaks; removal of an exported thumbnail-size helper; localization key updates for public lobbies and the mode selector. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerConfig as Server Config
participant MapService as Map/Terrain Loader
participant LobbyUI as Lobby Renderer
Client->>ServerConfig: getServerConfigFromClient()
ServerConfig-->>Client: config (defaultLobbyTime, timings)
Client->>Client: set defaultLobbyTime
Note over Client,LobbyUI: On public lobbies update
Client->>LobbyUI: receive lobby list
LobbyUI->>MapService: request map dimensions for each lobby map
MapService-->>LobbyUI: return width,height (aspect)
LobbyUI->>Client: store mapAspectRatios
Client->>LobbyUI: render lobby cards using mapAspectRatios & defaultLobbyTime
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/GameModeSelector.ts`:
- Line 33: The defaultLobbyTime field is set to "-" which leads to rendering
"-s" when config is slow/fails; update defaultLobbyTime to an empty string or
null and change the render/formatting logic (where the code appends "s"—refer to
the places around lines that modify/display lobby time) to only append "s" when
the value is a valid number; also add a .catch (or try/catch) to the
config-fetching function (the method that requests lobby/config data in this
file) to set the fallback label (empty string or a human-friendly placeholder)
on failure so rejected promises are handled and no "-s" is shown.
- Around line 91-107: The map manifest requests are duplicated because
concurrent calls to the block in GameModeSelector (inside handleLobbiesUpdate or
equivalent) trigger multiple terrainMapFileLoader.getMapData(mapType).manifest()
for the same mapType; add an in-flight tracker (e.g., a Set or Map named
mapManifestInFlight or mapManifestPromises) keyed by mapType, check it before
calling getMapData().manifest(), store the Promise when starting the request,
and on resolution set this.mapAspectRatios (as done now) and remove the key from
the tracker; also ensure the catch handler removes the key on failure so future
retries can proceed.
- Around line 124-158: The primary lobby card is being duplicated on mobile
because the top conditional block (the ${special ? html`<div
class="lg:row-span-2">...` : ffa ? html`<div class="lg:row-span-2">...` :
nothing} fragment) is visible on all breakpoints while you also render separate
lg:hidden mobile cards; to fix, make that top primary-card container only appear
on large screens by adding responsive visibility classes (e.g., add "hidden
lg:block" or similar) to the outer divs that wrap the primary render (the divs
produced in the ternary that call renderSpecialLobbyCard(special) or
renderLobbyCard(ffa, this.getLobbyTitle(ffa))) so they are hidden on mobile and
only shown on lg and up, leaving the existing lg:hidden mobile blocks unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2347414-f235-4b8e-a784-bb1224d25055
📒 Files selected for processing (10)
index.htmlresources/lang/en.jsonsrc/client/GameInfoModal.tssrc/client/GameModeSelector.tssrc/client/GutterAds.tssrc/client/components/DesktopNavBar.tssrc/client/components/Footer.tssrc/client/components/MainLayout.tssrc/client/styles/layout/header.csssrc/core/game/Game.ts
💤 Files with no reviewable changes (2)
- src/core/game/Game.ts
- src/client/styles/layout/header.css
Comment on lines
+91
to
+107
| const allGames = Object.values(lobbies.games ?? {}).flat(); | ||
| for (const game of allGames) { | ||
| const mapType = game.gameConfig?.gameMap as GameMapType; | ||
| if (mapType && !this.mapAspectRatios.has(mapType)) { | ||
| terrainMapFileLoader | ||
| .getMapData(mapType) | ||
| .manifest() | ||
| .then((m: any) => { | ||
| if (m?.map?.width && m?.map?.height) { | ||
| this.mapAspectRatios = new Map(this.mapAspectRatios).set( | ||
| mapType, | ||
| m.map.width / m.map.height, | ||
| ); | ||
| } | ||
| }) | ||
| .catch(() => {}); | ||
| } |
Contributor
There was a problem hiding this comment.
Deduplicate map manifest requests while loads are in-flight.
handleLobbiesUpdate can run repeatedly before the first manifest() resolves, causing repeated fetches for the same map.
💡 Proposed fix
`@state`() private mapAspectRatios: Map<GameMapType, number> = new Map();
+ private pendingMapAspectRatios: Set<GameMapType> = new Set();
@@
- if (mapType && !this.mapAspectRatios.has(mapType)) {
+ if (
+ mapType &&
+ !this.mapAspectRatios.has(mapType) &&
+ !this.pendingMapAspectRatios.has(mapType)
+ ) {
+ this.pendingMapAspectRatios.add(mapType);
terrainMapFileLoader
.getMapData(mapType)
.manifest()
.then((m: any) => {
if (m?.map?.width && m?.map?.height) {
this.mapAspectRatios = new Map(this.mapAspectRatios).set(
mapType,
m.map.width / m.map.height,
);
}
})
- .catch(() => {});
+ .catch(() => {})
+ .finally(() => {
+ this.pendingMapAspectRatios.delete(mapType);
+ });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/GameModeSelector.ts` around lines 91 - 107, The map manifest
requests are duplicated because concurrent calls to the block in
GameModeSelector (inside handleLobbiesUpdate or equivalent) trigger multiple
terrainMapFileLoader.getMapData(mapType).manifest() for the same mapType; add an
in-flight tracker (e.g., a Set or Map named mapManifestInFlight or
mapManifestPromises) keyed by mapType, check it before calling
getMapData().manifest(), store the Promise when starting the request, and on
resolution set this.mapAspectRatios (as done now) and remove the key from the
tracker; also ensure the catch handler removes the key on failure so future
retries can proceed.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/client/GameModeSelector.ts (3)
91-107:⚠️ Potential issue | 🟠 MajorDeduplicate map manifest fetches while requests are in flight.
Lines 91-107 can trigger repeated
manifest()calls for the same map before the first request finishes.💡 Proposed fix
`@state`() private mapAspectRatios: Map<GameMapType, number> = new Map(); + private pendingMapAspectRatios: Set<GameMapType> = new Set(); @@ - if (mapType && !this.mapAspectRatios.has(mapType)) { + if ( + mapType && + !this.mapAspectRatios.has(mapType) && + !this.pendingMapAspectRatios.has(mapType) + ) { + this.pendingMapAspectRatios.add(mapType); terrainMapFileLoader .getMapData(mapType) .manifest() @@ - .catch(() => {}); + .catch(() => {}) + .finally(() => { + this.pendingMapAspectRatios.delete(mapType); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` around lines 91 - 107, The loop in GameModeSelector is causing duplicate terrainMapFileLoader.getMapData(mapType).manifest() calls because it only checks this.mapAspectRatios; fix by introducing an in-flight promise map (e.g., this.mapManifestPromises) and, before calling manifest(), check if a promise for mapType exists and reuse it; when creating the request store the returned Promise there, on success set this.mapAspectRatios.set(mapType, width/height) and delete the entry from this.mapManifestPromises, and on error delete the entry as well so subsequent attempts can retry; ensure all references use the same mapType key and that existing code still reads from this.mapAspectRatios.
124-132:⚠️ Potential issue | 🟠 MajorPrimary lobby card is duplicated on mobile.
The top primary card block is visible on mobile, and mobile blocks also render
ffa/special, so the same card can appear twice.💡 Proposed fix
- ${special - ? html`<div class="sm:row-span-2"> + ${special + ? html`<div class="hidden sm:block sm:row-span-2"> ${this.renderSpecialLobbyCard(special)} </div>` : ffa - ? html`<div class="sm:row-span-2"> + ? html`<div class="hidden sm:block sm:row-span-2"> ${this.renderLobbyCard(ffa, this.getLobbyTitle(ffa))} </div>` : nothing}Also applies to: 146-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` around lines 124 - 132, The primary lobby card is rendered twice because the top card block (which uses renderSpecialLobbyCard / renderLobbyCard + getLobbyTitle) is visible on mobile while the mobile-specific blocks also render ffa/special; fix by making the top primary block hidden on small screens (or vice-versa). Concretely, update the template that calls renderSpecialLobbyCard / renderLobbyCard (the snippet with class "sm:row-span-2") to include a responsive visibility utility (e.g., add "hidden sm:block" to that container) so the primary card only appears on sm+ screens, or alternatively wrap the mobile blocks in "sm:hidden" so they don’t duplicate the top card; adjust the same change in the similar block around renderLobbyCard/renderSpecialLobbyCard at lines 146-158.
33-33:⚠️ Potential issue | 🟡 MinorHandle config fetch failure so the timer never shows
-s.Line 67 has no rejection handling, and Line 265 can render
-swhen config fetch fails or is slow.💡 Proposed fix
- private defaultLobbyTime: string = "-"; + private defaultLobbyTime: number | null = null; @@ - getServerConfigFromClient().then((config) => { - this.defaultLobbyTime = (config.gameCreationRate() / 1000).toString(); - }); + void getServerConfigFromClient() + .then((config) => { + this.defaultLobbyTime = Math.floor(config.gameCreationRate() / 1000); + }) + .catch(() => { + this.defaultLobbyTime = null; + }); @@ - if (timeRemaining === undefined) { - timeDisplay = this.defaultLobbyTime + "s"; + if (timeRemaining === undefined) { + timeDisplay = + this.defaultLobbyTime !== null ? `${this.defaultLobbyTime}s` : "—";Also applies to: 67-69, 264-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` at line 33, The lobby timer default value defaultLobbyTime = "-" is shown as "-s" when the config fetch fails or is slow; add rejection handling to the async config fetch that assigns a safe fallback (e.g., "0" or an empty string) to defaultLobbyTime on error, and update the render logic that appends "s" so it guards against the placeholder (only append "s" when defaultLobbyTime is a numeric value); specifically, add a .catch or try/catch around the function that loads the lobby config and sets defaultLobbyTime and ensure the component's render/format routine for the timer checks the value before concatenating "s".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/GameModeSelector.ts`:
- Around line 91-107: The loop in GameModeSelector is causing duplicate
terrainMapFileLoader.getMapData(mapType).manifest() calls because it only checks
this.mapAspectRatios; fix by introducing an in-flight promise map (e.g.,
this.mapManifestPromises) and, before calling manifest(), check if a promise for
mapType exists and reuse it; when creating the request store the returned
Promise there, on success set this.mapAspectRatios.set(mapType, width/height)
and delete the entry from this.mapManifestPromises, and on error delete the
entry as well so subsequent attempts can retry; ensure all references use the
same mapType key and that existing code still reads from this.mapAspectRatios.
- Around line 124-132: The primary lobby card is rendered twice because the top
card block (which uses renderSpecialLobbyCard / renderLobbyCard + getLobbyTitle)
is visible on mobile while the mobile-specific blocks also render ffa/special;
fix by making the top primary block hidden on small screens (or vice-versa).
Concretely, update the template that calls renderSpecialLobbyCard /
renderLobbyCard (the snippet with class "sm:row-span-2") to include a responsive
visibility utility (e.g., add "hidden sm:block" to that container) so the
primary card only appears on sm+ screens, or alternatively wrap the mobile
blocks in "sm:hidden" so they don’t duplicate the top card; adjust the same
change in the similar block around renderLobbyCard/renderSpecialLobbyCard at
lines 146-158.
- Line 33: The lobby timer default value defaultLobbyTime = "-" is shown as "-s"
when the config fetch fails or is slow; add rejection handling to the async
config fetch that assigns a safe fallback (e.g., "0" or an empty string) to
defaultLobbyTime on error, and update the render logic that appends "s" so it
guards against the placeholder (only append "s" when defaultLobbyTime is a
numeric value); specifically, add a .catch or try/catch around the function that
loads the lobby config and sets defaultLobbyTime and ensure the component's
render/format routine for the timer checks the value before concatenating "s".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a74e9c1d-16d2-45ea-abac-e6aebf5f9efd
📒 Files selected for processing (10)
index.htmlresources/lang/en.jsonsrc/client/GameInfoModal.tssrc/client/GameModeSelector.tssrc/client/GutterAds.tssrc/client/components/DesktopNavBar.tssrc/client/components/Footer.tssrc/client/components/MainLayout.tssrc/client/styles/layout/header.csssrc/core/game/Game.ts
💤 Files with no reviewable changes (2)
- src/client/styles/layout/header.css
- src/core/game/Game.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/GutterAds.ts
- src/client/components/DesktopNavBar.ts
- src/client/components/Footer.ts
- src/client/components/MainLayout.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/GameModeSelector.ts (1)
33-33:⚠️ Potential issue | 🟡 MinorHandle config fetch failure to avoid showing
-s.When
getServerConfigFromClient()is slow or fails,defaultLobbyTimestays as"-"and the UI shows"-s". The promise rejection is also unhandled.💡 Proposed fix
- private defaultLobbyTime: string = "-"; + private defaultLobbyTime: string | null = null;Then update the display logic at line 265:
if (timeRemaining === undefined) { - timeDisplay = this.defaultLobbyTime + "s"; + timeDisplay = this.defaultLobbyTime !== null ? `${this.defaultLobbyTime}s` : "—"; }And add error handling at lines 67-69:
- getServerConfigFromClient().then((config) => { - this.defaultLobbyTime = (config.gameCreationRate() / 1000).toString(); - }); + getServerConfigFromClient() + .then((config) => { + this.defaultLobbyTime = Math.floor(config.gameCreationRate() / 1000).toString(); + }) + .catch(() => { + this.defaultLobbyTime = null; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` at line 33, The defaultLobbyTime field currently initialized to "-" causes the UI to render "-s" and leaves promise rejections unhandled; change defaultLobbyTime (in GameModeSelector) to null (or empty string) instead of "-" and update the display logic that appends "s" to render nothing when defaultLobbyTime is null/empty (avoid concatenating "s" to "-"); also add proper error handling on the getServerConfigFromClient() promise (add a .catch that sets defaultLobbyTime to null/empty and logs the error) so rejections are handled and the UI does not show stray "-" characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/GameModeSelector.ts`:
- Line 248: The code uses a non-null assertion on lobby.gameConfig (const
mapType = lobby.gameConfig!.gameMap as GameMapType) which can throw if
gameConfig is undefined; update the surrounding logic in GameModeSelector.ts to
guard against a missing gameConfig by returning early or providing a sensible
fallback before accessing gameConfig.gameMap—e.g., check if lobby.gameConfig is
truthy (or use optional chaining) and handle the undefined case (early return,
default mapType, or disabled UI path) so mapType is only read when gameConfig
exists.
---
Duplicate comments:
In `@src/client/GameModeSelector.ts`:
- Line 33: The defaultLobbyTime field currently initialized to "-" causes the UI
to render "-s" and leaves promise rejections unhandled; change defaultLobbyTime
(in GameModeSelector) to null (or empty string) instead of "-" and update the
display logic that appends "s" to render nothing when defaultLobbyTime is
null/empty (avoid concatenating "s" to "-"); also add proper error handling on
the getServerConfigFromClient() promise (add a .catch that sets defaultLobbyTime
to null/empty and logs the error) so rejections are handled and the UI does not
show stray "-" characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c2011bb-d4ee-4b91-86c9-529e90c3fdbd
📒 Files selected for processing (10)
index.htmlresources/lang/en.jsonsrc/client/GameInfoModal.tssrc/client/GameModeSelector.tssrc/client/GutterAds.tssrc/client/components/DesktopNavBar.tssrc/client/components/Footer.tssrc/client/components/MainLayout.tssrc/client/styles/layout/header.csssrc/core/game/Game.ts
💤 Files with no reviewable changes (2)
- src/client/styles/layout/header.css
- src/core/game/Game.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/components/Footer.ts
- src/client/components/DesktopNavBar.ts
- resources/lang/en.json
ryanbarlow97
requested changes
Mar 5, 2026
| > | ||
| <div | ||
| class="w-full lg:max-w-[24cm] mx-auto flex flex-col flex-1 gap-0 lg:gap-[clamp(1.5rem,3vw,3rem)] overflow-y-auto overflow-x-hidden" | ||
| class="w-full lg:max-w-[19cm] mx-auto flex flex-col flex-1 gap-0 lg:gap-[clamp(1.5rem,3vw,3rem)] overflow-y-auto overflow-x-hidden" |
Contributor
There was a problem hiding this comment.
it was 20cm before my change to 24cm
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/client/GameModeSelector.ts (4)
33-33:⚠️ Potential issue | 🟡 MinorAvoid rendering
-sand handle config fetch failure.
defaultLobbyTimestarts as"-", and whentimeRemaining === undefinedyou render${defaultLobbyTime}s, which can show-s. AlsogetServerConfigFromClient()can reject and the promise is currently unhandled.Proposed fix
- private defaultLobbyTime: string = "-"; + private defaultLobbyTime: number | null = null; connectedCallback() { super.connectedCallback(); this.lobbySocket.start(); - getServerConfigFromClient().then((config) => { - this.defaultLobbyTime = (config.gameCreationRate() / 1000).toString(); - }); + void getServerConfigFromClient() + .then((config) => { + this.defaultLobbyTime = Math.floor(config.gameCreationRate() / 1000); + }) + .catch(() => { + this.defaultLobbyTime = null; + }); } @@ - if (timeRemaining === undefined) { - timeDisplay = this.defaultLobbyTime + "s"; + if (timeRemaining === undefined) { + timeDisplay = + this.defaultLobbyTime !== null ? `${this.defaultLobbyTime}s` : "—"; } else if (timeRemaining > 0) {Also applies to: 67-69, 258-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` at line 33, defaultLobbyTime is initialized to "-" and when timeRemaining === undefined the UI renders `${defaultLobbyTime}s` producing "-s", and getServerConfigFromClient() calls are not handling rejections; change defaultLobbyTime to a non-placeholder (e.g., undefined or an empty string) and update rendering logic around timeRemaining to only append "s" when a numeric value exists (or use a safe numeric fallback like 0), and wrap all getServerConfigFromClient() invocations in try/catch (or attach .catch) to handle promise rejection and set sensible defaults for related fields; target the symbols defaultLobbyTime, timeRemaining, and getServerConfigFromClient in your edits and apply the same pattern at the other occurrences noted (lines ~67-69 and ~258-265).
238-247:⚠️ Potential issue | 🟡 MinorRemove
gameConfig!non-null assertions (still a crash risk).
renderLobbyCard()andgetLobbyTitle()both assumelobby.gameConfig!exists. If the server ever sends a lobby withoutgameConfig(or it’s briefly undefined during updates), this will throw and break the whole page.Proposed fix
private renderLobbyCard( lobby: PublicGameInfo, titleContent: string | TemplateResult, ) { - const mapType = lobby.gameConfig!.gameMap as GameMapType; + if (!lobby.gameConfig) { + return nothing; + } + const mapType = lobby.gameConfig.gameMap as GameMapType; const mapImageSrc = terrainMapFileLoader.getMapData(mapType).webpPath; @@ const mapName = getMapName(lobby.gameConfig?.gameMap);private getLobbyTitle(lobby: PublicGameInfo): string { - const config = lobby.gameConfig!; + const config = lobby.gameConfig; + if (!config) return "";Also applies to: 368-371
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` around lines 238 - 247, renderLobbyCard and getLobbyTitle currently use the non-null assertion lobby.gameConfig! which can crash if gameConfig is missing; change both functions (renderLobbyCard, getLobbyTitle) to safely handle undefined gameConfig by removing the `!` and adding a guard (e.g., if (!lobby.gameConfig) return a safe fallback UI or title) or by using optional chaining/default values when reading properties (e.g., lobby.gameConfig?.gameMap) and fallbacks for mapImageSrc/aspectRatio/timeRemaining; also apply the same guard to the other occurrences referenced in getLobbyTitle to prevent runtime exceptions from missing gameConfig.
116-165:⚠️ Potential issue | 🟠 MajorMobile currently renders duplicate lobby cards.
On mobile (
sm:hiddenblocks active), you still render the “main card” (special/ffa) at the top of the grid and render the dedicated mobile cards for ffa/special below—so the same lobby can appear twice.A simple fix is to hide the “main card” block on mobile and keep the explicit mobile list, or remove the explicit mobile duplicates.
One possible fix (hide the main card on mobile)
- ${special - ? html`${this.renderSpecialLobbyCard(special)}` - : ffa - ? html`${this.renderLobbyCard(ffa, this.getLobbyTitle(ffa))}` - : nothing} + <div class="hidden sm:block"> + ${special + ? html`${this.renderSpecialLobbyCard(special)}` + : ffa + ? html`${this.renderLobbyCard(ffa, this.getLobbyTitle(ffa))}` + : nothing} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` around lines 116 - 165, The main lobby card (renderSpecialLobbyCard/renderLobbyCard using special/ffa) is rendered both in the grid's left column and again in the mobile-specific blocks, causing duplicates on mobile; modify the grid-left main card rendering (the conditional that uses special ? this.renderSpecialLobbyCard(special) : ffa ? this.renderLobbyCard(ffa, this.getLobbyTitle(ffa)) : nothing) so it is hidden on small screens (e.g., wrap it in a container with "hidden sm:block" or add "sm:hidden" logic inverted), or alternatively remove the mobile-specific duplicate blocks; update the condition around special/ffa in GameModeSelector so only one of renderSpecialLobbyCard/renderLobbyCard is output on mobile.
91-107:⚠️ Potential issue | 🟠 MajorDeduplicate
manifest()loads while requests are in-flight.
handleLobbiesUpdate()can fire multiple times before a givenmanifest()resolves. The!this.mapAspectRatios.has(mapType)check won’t prevent concurrent duplicate fetches for the samemapType.Proposed fix
`@state`() private mapAspectRatios: Map<GameMapType, number> = new Map(); + private pendingMapAspectRatios: Set<GameMapType> = new Set(); @@ - if (mapType && !this.mapAspectRatios.has(mapType)) { + if ( + mapType && + !this.mapAspectRatios.has(mapType) && + !this.pendingMapAspectRatios.has(mapType) + ) { + this.pendingMapAspectRatios.add(mapType); terrainMapFileLoader .getMapData(mapType) .manifest() .then((m: any) => { if (m?.map?.width && m?.map?.height) { this.mapAspectRatios = new Map(this.mapAspectRatios).set( mapType, m.map.width / m.map.height, ); } }) - .catch(() => {}); + .catch(() => {}) + .finally(() => { + this.pendingMapAspectRatios.delete(mapType); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` around lines 91 - 107, The code can trigger duplicate manifest() loads because the existing check uses only this.mapAspectRatios; to fix, introduce an in-flight promises cache (e.g., this.mapManifestPromises: Map<GameMapType, Promise<void>>), check it before calling terrainMapFileLoader.getMapData(mapType).manifest(), and if absent create and store a promise that calls manifest(), computes m.map.width / m.map.height, updates this.mapAspectRatios.set(mapType, ratio), and finally removes the entry from this.mapManifestPromises in both then and catch; replace the existing direct manifest() call in the loop with this deduplicated promise logic so concurrent handleLobbiesUpdate() invocations reuse the same in-flight request.
🧹 Nitpick comments (2)
src/client/GameInfoModal.ts (1)
4-4: Alwaysobject-covercan crop extreme-aspect thumbnails; addaltif possible.Now that the image is always
object-cover object-center, very wide/tall maps may get cropped more than before. If that’s intentional, fine—otherwise consider reintroducing a lightweight “contain for extreme ratios” check (similar to whatGameModeSelectordoes) to avoid losing important parts of the map preview.Also, consider adding an
altattribute for basic accessibility (even if it’s just${info.config.gameMap}).Also applies to: 115-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameInfoModal.ts` at line 4, The map thumbnail in GameInfoModal currently always uses "object-cover object-center", which can crop extreme-aspect previews; update the rendering logic in the GameInfoModal component to detect extreme aspect ratios (reusing the same lightweight "contain for extreme ratios" approach used in GameModeSelector) and switch the image class between "object-cover" and "object-contain" accordingly to avoid losing important map content; also add an alt attribute (e.g., using info.config.gameMap or a fallback string) to the <img> used for the map preview (this applies to the image render locations in GameInfoModal, including the block around lines 115-121).src/client/GutterAds.ts (1)
123-125: z-index drop toz-40looks correct; consider making positioning less fragile.Putting gutter ads at
z-40should keep them behind modal/page overlays (z-50) and persistent UI, which matches the layering intent.The
left: calc(50% ± 10cm ± Npx)approach is still quite sensitive to zoom/device metrics; if you see alignment drift on common breakpoints, consider anchoring to a container width (or usingright:for the right ad) rather than mixingcmandpx.Also applies to: 134-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GutterAds.ts` around lines 123 - 125, GutterAds uses fragile inline positioning (style="left: calc(50% - 10cm - 208px);") which can drift with zoom/device metrics; update the positioning in the GutterAds markup to anchor to a predictable container/viewport metric instead of mixing cm and px — e.g., compute offset from the centered content width or use right: calc((100vw - var(--content-width))/2 + <offset>) or use vw units (or CSS variables like --content-width) so the ad remains aligned; replace the inline left calc on the left/right ad elements in the GutterAds component with the new calc or right-based formula and adjust the mirrored instance (lines around 134-136) similarly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/GameModeSelector.ts`:
- Line 33: defaultLobbyTime is initialized to "-" and when timeRemaining ===
undefined the UI renders `${defaultLobbyTime}s` producing "-s", and
getServerConfigFromClient() calls are not handling rejections; change
defaultLobbyTime to a non-placeholder (e.g., undefined or an empty string) and
update rendering logic around timeRemaining to only append "s" when a numeric
value exists (or use a safe numeric fallback like 0), and wrap all
getServerConfigFromClient() invocations in try/catch (or attach .catch) to
handle promise rejection and set sensible defaults for related fields; target
the symbols defaultLobbyTime, timeRemaining, and getServerConfigFromClient in
your edits and apply the same pattern at the other occurrences noted (lines
~67-69 and ~258-265).
- Around line 238-247: renderLobbyCard and getLobbyTitle currently use the
non-null assertion lobby.gameConfig! which can crash if gameConfig is missing;
change both functions (renderLobbyCard, getLobbyTitle) to safely handle
undefined gameConfig by removing the `!` and adding a guard (e.g., if
(!lobby.gameConfig) return a safe fallback UI or title) or by using optional
chaining/default values when reading properties (e.g.,
lobby.gameConfig?.gameMap) and fallbacks for
mapImageSrc/aspectRatio/timeRemaining; also apply the same guard to the other
occurrences referenced in getLobbyTitle to prevent runtime exceptions from
missing gameConfig.
- Around line 116-165: The main lobby card
(renderSpecialLobbyCard/renderLobbyCard using special/ffa) is rendered both in
the grid's left column and again in the mobile-specific blocks, causing
duplicates on mobile; modify the grid-left main card rendering (the conditional
that uses special ? this.renderSpecialLobbyCard(special) : ffa ?
this.renderLobbyCard(ffa, this.getLobbyTitle(ffa)) : nothing) so it is hidden on
small screens (e.g., wrap it in a container with "hidden sm:block" or add
"sm:hidden" logic inverted), or alternatively remove the mobile-specific
duplicate blocks; update the condition around special/ffa in GameModeSelector so
only one of renderSpecialLobbyCard/renderLobbyCard is output on mobile.
- Around line 91-107: The code can trigger duplicate manifest() loads because
the existing check uses only this.mapAspectRatios; to fix, introduce an
in-flight promises cache (e.g., this.mapManifestPromises: Map<GameMapType,
Promise<void>>), check it before calling
terrainMapFileLoader.getMapData(mapType).manifest(), and if absent create and
store a promise that calls manifest(), computes m.map.width / m.map.height,
updates this.mapAspectRatios.set(mapType, ratio), and finally removes the entry
from this.mapManifestPromises in both then and catch; replace the existing
direct manifest() call in the loop with this deduplicated promise logic so
concurrent handleLobbiesUpdate() invocations reuse the same in-flight request.
---
Nitpick comments:
In `@src/client/GameInfoModal.ts`:
- Line 4: The map thumbnail in GameInfoModal currently always uses "object-cover
object-center", which can crop extreme-aspect previews; update the rendering
logic in the GameInfoModal component to detect extreme aspect ratios (reusing
the same lightweight "contain for extreme ratios" approach used in
GameModeSelector) and switch the image class between "object-cover" and
"object-contain" accordingly to avoid losing important map content; also add an
alt attribute (e.g., using info.config.gameMap or a fallback string) to the
<img> used for the map preview (this applies to the image render locations in
GameInfoModal, including the block around lines 115-121).
In `@src/client/GutterAds.ts`:
- Around line 123-125: GutterAds uses fragile inline positioning (style="left:
calc(50% - 10cm - 208px);") which can drift with zoom/device metrics; update the
positioning in the GutterAds markup to anchor to a predictable
container/viewport metric instead of mixing cm and px — e.g., compute offset
from the centered content width or use right: calc((100vw -
var(--content-width))/2 + <offset>) or use vw units (or CSS variables like
--content-width) so the ad remains aligned; replace the inline left calc on the
left/right ad elements in the GutterAds component with the new calc or
right-based formula and adjust the mirrored instance (lines around 134-136)
similarly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88bef6a5-2822-46f8-9b80-1c7be15c687f
📒 Files selected for processing (10)
index.htmlresources/lang/en.jsonsrc/client/GameInfoModal.tssrc/client/GameModeSelector.tssrc/client/GutterAds.tssrc/client/components/DesktopNavBar.tssrc/client/components/Footer.tssrc/client/components/MainLayout.tssrc/client/styles/layout/header.csssrc/core/game/Game.ts
💤 Files with no reviewable changes (2)
- src/client/styles/layout/header.css
- src/core/game/Game.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/components/Footer.ts
- src/client/components/MainLayout.ts
- src/client/components/DesktopNavBar.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/GutterAds.ts (1)
123-135:⚠️ Potential issue | 🟠 MajorMake ad stacking explicit vs alert overlay
Line 123 and Line 134 use
z-40, which matchessrc/client/graphics/layers/AlertFrame.ts(Line 33-45,z-index: 40). With equal z-index, draw order is DOM-order dependent, so ads can compete with the alert border visuals. Use a lower z-index for gutter ads to keep alerts consistently on top.Suggested fix
- class="hidden xl:flex fixed transform -translate-y-1/2 w-[160px] min-h-[600px] z-40 pointer-events-auto items-center justify-center" + class="hidden xl:flex fixed transform -translate-y-1/2 w-[160px] min-h-[600px] z-30 pointer-events-auto items-center justify-center" ... - class="hidden xl:flex fixed transform -translate-y-1/2 w-[160px] min-h-[600px] z-40 pointer-events-auto items-center justify-center" + class="hidden xl:flex fixed transform -translate-y-1/2 w-[160px] min-h-[600px] z-30 pointer-events-auto items-center justify-center"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GutterAds.ts` around lines 123 - 135, The gutter ad containers in src/client/GutterAds.ts use class "z-40" which ties their z-index to the AlertFrame overlay; change the gutter ad wrappers (the left container with id referenced by this.leftContainerId and the right gutter ad div) to a lower z-index (e.g., "z-30" or another value below AlertFrame's z-index) so alerts always render on top; update both occurrences of "z-40" in those ad divs to the chosen lower z-class.resources/lang/en.json (1)
363-376:⚠️ Potential issue | 🔴 CriticalThe
teams_Duos,teams_Trios, andteams_Quadskeys were removed from thepublic_lobbysection, breaking display labels.The code in
src/client/Utils.ts(lines 72-75) dynamically constructs keys likepublic_lobby.teams_Duos, but these keys no longer exist in thepublic_lobbysection ofen.json. The fallback logic will trigger and users will see numeric labels (e.g., "2 teams of 2") instead of friendly names (e.g., "Duos (teams of 2)").These team modes are still actively supported throughout the codebase—they exist in game config, map playlists, and UI components. Either restore the keys to the
public_lobbysection or update the code to use translation keys from their current location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/en.json` around lines 363 - 376, The i18n keys public_lobby.teams_Duos, public_lobby.teams_Trios and public_lobby.teams_Quads referenced by the dynamic key construction in src/client/Utils.ts (around the logic that builds `public_lobby.teams_${modeName}` at lines ~72-75) were removed, causing fallback numeric labels; fix by restoring these keys under the public_lobby object in en.json with the friendly labels (e.g., "Duos (teams of 2)", "Trios (teams of 3)", "Quads (teams of 4)"), or alternatively update the Utils.ts code to point to the current canonical translation keys for team-mode names instead of `public_lobby.teams_${...}` so the dynamic lookup resolves to existing keys.
♻️ Duplicate comments (3)
src/client/GameModeSelector.ts (3)
91-107:⚠️ Potential issue | 🟠 MajorDeduplicate map manifest requests while loads are in-flight.
Repeated lobby updates can trigger duplicate
manifest()requests for the same map before the first one completes.Proposed fix
`@state`() private mapAspectRatios: Map<GameMapType, number> = new Map(); + private pendingMapAspectRatios: Set<GameMapType> = new Set(); @@ - if (mapType && !this.mapAspectRatios.has(mapType)) { + if ( + mapType && + !this.mapAspectRatios.has(mapType) && + !this.pendingMapAspectRatios.has(mapType) + ) { + this.pendingMapAspectRatios.add(mapType); terrainMapFileLoader .getMapData(mapType) .manifest() @@ - .catch(() => {}); + .catch(() => {}) + .finally(() => { + this.pendingMapAspectRatios.delete(mapType); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` around lines 91 - 107, The loop in GameModeSelector that calls terrainMapFileLoader.getMapData(mapType).manifest() can start duplicate manifest() requests for the same map; add a simple in-flight-tracking Map (e.g., this.mapManifestPromises) keyed by mapType, check it before calling manifest(), store the returned promise when starting a request, and on both then and catch remove the entry and update this.mapAspectRatios (the existing setter logic in the then branch) so subsequent iterations reuse the pending promise and avoid duplicate network calls; reference the existing mapAspectRatios field and the terrainMapFileLoader.getMapData(...).manifest() call site to locate where to add the tracking.
131-135:⚠️ Potential issue | 🟠 MajorHide the primary top card on mobile to prevent duplicate cards.
The top primary card is visible on mobile, and mobile blocks also render FFA/special, so cards can show twice.
Proposed fix
- ${special - ? html`${this.renderSpecialLobbyCard(special)}` + ${special + ? html`<div class="hidden sm:block"> + ${this.renderSpecialLobbyCard(special)} + </div>` : ffa - ? html`${this.renderLobbyCard(ffa, this.getLobbyTitle(ffa))}` + ? html`<div class="hidden sm:block"> + ${this.renderLobbyCard(ffa, this.getLobbyTitle(ffa))} + </div>` : nothing}Also applies to: 157-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` around lines 131 - 135, The primary top card is duplicating on mobile because both the primary block and the FFA/special blocks render; update the rendering logic so the FFA/special cards are hidden on mobile when the primary top card is shown: in the template pieces that call renderSpecialLobbyCard(special) and renderLobbyCard(ffa, this.getLobbyTitle(ffa)) add a mobile check (or apply a "hide-on-mobile" CSS class) so those branches skip rendering when the primary top card is active on small screens; apply the same change to the other occurrence that uses renderSpecialLobbyCard/renderLobbyCard (the block referenced at lines 157-164) to ensure consistency.
33-33:⚠️ Potential issue | 🟡 MinorHandle config fetch failures and avoid showing
-s.This still renders
-son fallback and leaves rejected config fetches unhandled.Proposed fix
- private defaultLobbyTime: string = "-"; + private defaultLobbyTime: number | null = null; @@ - getServerConfigFromClient().then((config) => { - this.defaultLobbyTime = (config.gameCreationRate() / 1000).toString(); - }); + void getServerConfigFromClient() + .then((config) => { + this.defaultLobbyTime = Math.floor(config.gameCreationRate() / 1000); + }) + .catch(() => { + this.defaultLobbyTime = null; + }); @@ - if (timeRemaining === undefined) { - timeDisplay = this.defaultLobbyTime + "s"; + if (timeRemaining === undefined) { + timeDisplay = + this.defaultLobbyTime !== null ? `${this.defaultLobbyTime}s` : "—";Also applies to: 67-69, 258-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameModeSelector.ts` at line 33, The field defaultLobbyTime currently uses the string "-" and rejected config fetches are unhandled, so change defaultLobbyTime to an empty string or null (instead of "-"), add catch handlers to the config-fetching promises (the methods that set lobby time and other config values referenced around lines 67-69 and 258-260) to handle failures by setting safe fallback values (''/null) and logging the error, and update the render logic to conditionally display values only when they are non-empty (avoid rendering "-" placeholders). Ensure every promise returned by the config-fetch functions is awaited or has a .catch to prevent unhandled rejections and that the setters (the code that assigns to defaultLobbyTime and the other config fields) use these safe fallbacks.
🧹 Nitpick comments (1)
src/client/GameInfoModal.ts (1)
114-118: Consider adding aspect ratio detection to GameInfoModal for consistency.
GameModeSelector.ts(lines 244-246) conditionally appliesobject-containfor maps with aspect ratio >4 or <0.25 to prevent cropping.GameInfoModal.tscurrently always usesobject-cover object-center, which crops unusual aspect ratio maps.The different visual design (GameInfoModal uses a mask gradient overlay) may explain this intentional difference. If the cropping serves your design goals, no change needed. Otherwise, consider adding the same aspect ratio detection logic for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/GameInfoModal.ts` around lines 114 - 118, GameInfoModal currently renders the map image with fixed "object-cover object-center" which will crop extreme aspect ratios; update the rendering in GameInfoModal (where mapImage is used inside the template) to detect the map aspect ratio (same logic used in GameModeSelector for >4 or <0.25) and conditionally apply "object-contain" instead of "object-cover" for those extreme ratios so maps aren’t cropped unexpectedly while preserving the existing mask/overlay styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@resources/lang/en.json`:
- Around line 363-376: The i18n keys public_lobby.teams_Duos,
public_lobby.teams_Trios and public_lobby.teams_Quads referenced by the dynamic
key construction in src/client/Utils.ts (around the logic that builds
`public_lobby.teams_${modeName}` at lines ~72-75) were removed, causing fallback
numeric labels; fix by restoring these keys under the public_lobby object in
en.json with the friendly labels (e.g., "Duos (teams of 2)", "Trios (teams of
3)", "Quads (teams of 4)"), or alternatively update the Utils.ts code to point
to the current canonical translation keys for team-mode names instead of
`public_lobby.teams_${...}` so the dynamic lookup resolves to existing keys.
In `@src/client/GutterAds.ts`:
- Around line 123-135: The gutter ad containers in src/client/GutterAds.ts use
class "z-40" which ties their z-index to the AlertFrame overlay; change the
gutter ad wrappers (the left container with id referenced by
this.leftContainerId and the right gutter ad div) to a lower z-index (e.g.,
"z-30" or another value below AlertFrame's z-index) so alerts always render on
top; update both occurrences of "z-40" in those ad divs to the chosen lower
z-class.
---
Duplicate comments:
In `@src/client/GameModeSelector.ts`:
- Around line 91-107: The loop in GameModeSelector that calls
terrainMapFileLoader.getMapData(mapType).manifest() can start duplicate
manifest() requests for the same map; add a simple in-flight-tracking Map (e.g.,
this.mapManifestPromises) keyed by mapType, check it before calling manifest(),
store the returned promise when starting a request, and on both then and catch
remove the entry and update this.mapAspectRatios (the existing setter logic in
the then branch) so subsequent iterations reuse the pending promise and avoid
duplicate network calls; reference the existing mapAspectRatios field and the
terrainMapFileLoader.getMapData(...).manifest() call site to locate where to add
the tracking.
- Around line 131-135: The primary top card is duplicating on mobile because
both the primary block and the FFA/special blocks render; update the rendering
logic so the FFA/special cards are hidden on mobile when the primary top card is
shown: in the template pieces that call renderSpecialLobbyCard(special) and
renderLobbyCard(ffa, this.getLobbyTitle(ffa)) add a mobile check (or apply a
"hide-on-mobile" CSS class) so those branches skip rendering when the primary
top card is active on small screens; apply the same change to the other
occurrence that uses renderSpecialLobbyCard/renderLobbyCard (the block
referenced at lines 157-164) to ensure consistency.
- Line 33: The field defaultLobbyTime currently uses the string "-" and rejected
config fetches are unhandled, so change defaultLobbyTime to an empty string or
null (instead of "-"), add catch handlers to the config-fetching promises (the
methods that set lobby time and other config values referenced around lines
67-69 and 258-260) to handle failures by setting safe fallback values (''/null)
and logging the error, and update the render logic to conditionally display
values only when they are non-empty (avoid rendering "-" placeholders). Ensure
every promise returned by the config-fetch functions is awaited or has a .catch
to prevent unhandled rejections and that the setters (the code that assigns to
defaultLobbyTime and the other config fields) use these safe fallbacks.
---
Nitpick comments:
In `@src/client/GameInfoModal.ts`:
- Around line 114-118: GameInfoModal currently renders the map image with fixed
"object-cover object-center" which will crop extreme aspect ratios; update the
rendering in GameInfoModal (where mapImage is used inside the template) to
detect the map aspect ratio (same logic used in GameModeSelector for >4 or
<0.25) and conditionally apply "object-contain" instead of "object-cover" for
those extreme ratios so maps aren’t cropped unexpectedly while preserving the
existing mask/overlay styling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 094a45d1-f6cc-464d-a44d-2722a5a957a2
📒 Files selected for processing (11)
index.htmlresources/lang/en.jsonsrc/client/GameInfoModal.tssrc/client/GameModeSelector.tssrc/client/GutterAds.tssrc/client/components/DesktopNavBar.tssrc/client/components/Footer.tssrc/client/components/MainLayout.tssrc/client/components/PlayPage.tssrc/client/styles/layout/header.csssrc/core/game/Game.ts
💤 Files with no reviewable changes (2)
- src/core/game/Game.ts
- src/client/styles/layout/header.css
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/components/DesktopNavBar.ts
- src/client/components/MainLayout.ts
- index.html
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description:
A bunch of small UI improvements:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME