Add Multiple Lobby Support with Carousel Navigation #2019
Conversation
|
|
WalkthroughReplaces the single lobby view with a 3-card carousel with drag/swipe navigation, preloads lobby map images, updates click behavior to always join, and maintains selected lobby alignment. On the server, adds per-lobby failure tracking, throttled scheduling with cooldown, a cap of 3 public lobbies, and revised removal after 3 consecutive fetch failures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant PL as PublicLobby (client)
participant EB as EventBus
participant AS as AssetLoader
U->>PL: Drag start (mouse/touch)
PL->>PL: Track startX/currentX, set isDragging
U->>PL: Drag move
PL->>PL: Update dragOffset (visual slide)
U->>PL: Drag end
alt Exceeds threshold
PL->>PL: previousLobby()/nextLobby()
PL->>PL: Update currentLobbyIndex
else Not exceeded
U->>PL: Click lobby card
PL->>PL: handleLobbyClick (if !hasDragged)
PL->>EB: emit "join-lobby" with lobby info
end
rect rgba(200,255,200,0.2)
note over PL,AS: On lobby fetch/update
PL->>AS: loadMapImage(gameID, map)
AS-->>PL: Cache image, update render
end
sequenceDiagram
autonumber
participant S as Scheduler (server)
participant L as LobbyStore
participant G as GameFetch
participant Log as Logger
loop every 300ms
S->>S: if time since lastScheduleTime >= cooldown
S->>L: count public lobbies
alt lobbies < 3
S->>G: fetch/update public lobbies
alt success
G-->>S: lobbies data
S->>S: reset failCount for lobby GameID
S->>Log: info "scheduled/updated"
else failure
G-->>S: error
S->>S: increment failCount[GameID]
alt failCount >= 3
S->>L: remove lobby(GameID)
S->>Log: warn "removed after consecutive failures"
else
S->>Log: debug "retry later"
end
end
S->>S: update lastScheduleTime
else skip
S->>Log: debug "cooldown"
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/server/Master.ts (2)
103-104: Consider using const for immutable value
lastScheduleTimeis only reassigned, never mutated. Consider usingconstwith an object or wrapping in a closure for better immutability.- // Track when we last scheduled a lobby to prevent spam - let lastScheduleTime = 0; + // Track when we last scheduled a lobby to prevent spam + const scheduleState = { lastTime: 0 };Then update line 120:
- lastScheduleTime = now; + scheduleState.lastTime = now;And line 113:
- lobbies < 3 && now - lastScheduleTime > cooldownTime; + lobbies < 3 && now - scheduleState.lastTime > cooldownTime;
259-281: Consider extracting lobby cleanup logicThe lobby removal logic has two complex conditions that could be clearer as a separate function.
+ private shouldRemoveLobby(lobby: GameInfo): boolean { + // Remove if game is about to start + if (lobby.msUntilStart !== undefined && lobby.msUntilStart <= 250) { + return true; + } + + // Remove if lobby is full + if ( + lobby.gameConfig?.maxPlayers !== undefined && + lobby.numClients !== undefined && + lobby.gameConfig.maxPlayers <= lobby.numClients + ) { + return true; + } + + return false; + } lobbyInfos.forEach((l) => { - if ( - "msUntilStart" in l && - l.msUntilStart !== undefined && - l.msUntilStart <= 250 - ) { - publicLobbyIDs.delete(l.gameID); - return; - } - - if ( - "gameConfig" in l && - l.gameConfig !== undefined && - "maxPlayers" in l.gameConfig && - l.gameConfig.maxPlayers !== undefined && - "numClients" in l && - l.numClients !== undefined && - l.gameConfig.maxPlayers <= l.numClients - ) { + if (this.shouldRemoveLobby(l)) { publicLobbyIDs.delete(l.gameID); - return; } });src/client/PublicLobby.ts (2)
134-139: Consider extracting carousel transform calculationThe inline style calculation is complex. Consider extracting to a method for clarity.
+ private getCarouselTransform(): string { + const baseOffset = -this.currentLobbyIndex * 100; + return `translateX(calc(${baseOffset}% + ${this.dragOffset}px))`; + } <div class="flex ${this.isDragging ? "" : "transition-transform duration-300 ease-in-out"}" - style="transform: translateX(calc(-${this.currentLobbyIndex * - 100}% + ${this.dragOffset}px))" + style="transform: ${this.getCarouselTransform()}" @mousedown=${this.handleMouseDown} @touchstart=${this.handleTouchStart} >
389-392: Consider using a more robust click preventionThe 100ms timeout might not be enough on slower devices. Consider tracking drag state more explicitly.
Instead of using a timeout, you could reset
hasDraggedsynchronously at the start of the next interaction:private handleMouseDown(e: MouseEvent) { this.isDragging = true; this.hasDragged = false; // ... rest of the code } private handleTouchStart(e: TouchEvent) { this.isDragging = true; this.hasDragged = false; // ... rest of the code } private handleDragEnd() { // ... existing drag end logic this.isDragging = false; this.dragOffset = 0; this.requestUpdate(); - - // Reset hasDragged after a short delay to prevent accidental clicks - setTimeout(() => { - this.hasDragged = false; - }, 100); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/PublicLobby.ts(8 hunks)src/server/Master.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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/PublicLobby.ts
🧬 Code graph analysis (2)
src/server/Master.ts (1)
src/core/Schemas.ts (2)
GameID(20-20)GameInfo(114-120)
src/client/PublicLobby.ts (3)
src/core/Schemas.ts (2)
GameInfo(114-120)GameID(20-20)src/core/Util.ts (1)
generateID(222-228)src/client/Main.ts (1)
JoinLobbyEvent(72-80)
🔇 Additional comments (7)
src/server/Master.ts (3)
64-64: Good addition of per-lobby fail trackingThe Map structure with typed GameID key is a clean way to track failures per lobby.
109-122: Clean throttling logic for lobby schedulingThe 3-lobby cap with cooldown-based throttling provides good rate limiting. The informative log message helps with debugging.
223-239: Well-structured retry logic with gradual backoffThe 3-strike failure handling prevents temporary network issues from immediately removing lobbies. The distinction between debug and warn log levels is appropriate.
src/client/PublicLobby.ts (4)
16-25: Good state management for carouselThe drag-related state properties are well-organized and clearly named. The separation between
hasDraggedandisDraggingproperly handles the click vs drag distinction.
76-86: Nice async image preloadingThe map image preloading improves user experience by loading assets before they're needed in the carousel.
107-108: Proper cleanup when stoppingGood practice to hide the component when stopped. This aligns with the established pattern of using direct DOM manipulation during game transitions.
277-300: Smart lobby tracking logicThe
followSelectedLobbymethod elegantly maintains the selected lobby position when the list changes. Good edge case handling when lobby is removed.
| private handleTouchStart(e: TouchEvent) { | ||
| this.isDragging = true; | ||
| this.startX = e.touches[0].clientX; | ||
| this.currentX = e.touches[0].clientX; | ||
|
|
||
| const handleTouchMove = (e: TouchEvent) => { | ||
| if (!this.isDragging) return; | ||
| this.currentX = e.touches[0].clientX; | ||
| }; | ||
|
|
||
| const handleTouchEnd = () => { | ||
| if (this.isDragging) { | ||
| this.handleDragEnd(); | ||
| } | ||
| document.removeEventListener("touchmove", handleTouchMove); | ||
| document.removeEventListener("touchend", handleTouchEnd); | ||
| }; | ||
|
|
||
| document.addEventListener("touchmove", handleTouchMove); | ||
| document.addEventListener("touchend", handleTouchEnd); | ||
| } |
There was a problem hiding this comment.
Missing drag offset tracking in touch handler
The touch handler doesn't update dragOffset or set hasDragged like the mouse handler does, which could cause inconsistent behavior on mobile.
private handleTouchStart(e: TouchEvent) {
this.isDragging = true;
+ this.hasDragged = false;
this.startX = e.touches[0].clientX;
this.currentX = e.touches[0].clientX;
+ this.dragOffset = 0;
const handleTouchMove = (e: TouchEvent) => {
if (!this.isDragging) return;
this.currentX = e.touches[0].clientX;
+ this.dragOffset = this.currentX - this.startX;
+
+ if (Math.abs(this.dragOffset) > 5) {
+ this.hasDragged = true;
+ }
+
+ this.requestUpdate();
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private handleTouchStart(e: TouchEvent) { | |
| this.isDragging = true; | |
| this.startX = e.touches[0].clientX; | |
| this.currentX = e.touches[0].clientX; | |
| const handleTouchMove = (e: TouchEvent) => { | |
| if (!this.isDragging) return; | |
| this.currentX = e.touches[0].clientX; | |
| }; | |
| const handleTouchEnd = () => { | |
| if (this.isDragging) { | |
| this.handleDragEnd(); | |
| } | |
| document.removeEventListener("touchmove", handleTouchMove); | |
| document.removeEventListener("touchend", handleTouchEnd); | |
| }; | |
| document.addEventListener("touchmove", handleTouchMove); | |
| document.addEventListener("touchend", handleTouchEnd); | |
| } | |
| private handleTouchStart(e: TouchEvent) { | |
| this.isDragging = true; | |
| this.hasDragged = false; | |
| this.startX = e.touches[0].clientX; | |
| this.currentX = e.touches[0].clientX; | |
| this.dragOffset = 0; | |
| const handleTouchMove = (e: TouchEvent) => { | |
| if (!this.isDragging) return; | |
| this.currentX = e.touches[0].clientX; | |
| this.dragOffset = this.currentX - this.startX; | |
| if (Math.abs(this.dragOffset) > 5) { | |
| this.hasDragged = true; | |
| } | |
| this.requestUpdate(); | |
| }; | |
| const handleTouchEnd = () => { | |
| if (this.isDragging) { | |
| this.handleDragEnd(); | |
| } | |
| document.removeEventListener("touchmove", handleTouchMove); | |
| document.removeEventListener("touchend", handleTouchEnd); | |
| }; | |
| document.addEventListener("touchmove", handleTouchMove); | |
| document.addEventListener("touchend", handleTouchEnd); | |
| } |
🤖 Prompt for AI Agents
In src/client/PublicLobby.ts around lines 351 to 371, the touch handlers set
startX/currentX but never update dragOffset or hasDragged like the mouse
handlers do; update handleTouchMove to compute this.currentX, set
this.dragOffset = this.currentX - this.startX, and mark this.hasDragged = true
when movement exceeds the same threshold used by mouse dragging (or always set
true if no threshold exists), and ensure handleTouchEnd leaves this.hasDragged
set before calling handleDragEnd and that event listeners are removed as
currently done.
|
I think there are many players who are waiting for specific maps. With the carousel they would have to constantly navigate through it to keep an eye on all lobbies 🤔 |
Description:
Modified the public lobby system to maintain up to 3 lobbies simultaneously instead of just 1, giving players more game options to choose from. Added a carousel interface to browse between available lobbies with manual navigation controls.
Changes Made:
Key Features:
Files Modified:
src/client/PublicLobby.ts- Added carousel functionality and navigationsrc/server/Master.ts- Updated lobby scheduling and error handlingPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
ark_unity2