Lobby Notification System (Audible Ping on Match) #2666
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a lobby notification feature: UI button and modal, a client-side manager that listens for lobby updates and settings, plays an optional beep for matching lobbies, persists preferences, wiring in Main, tests, and translations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Button as LobbyNotificationButton
participant Window
participant Modal as LobbyNotificationModal
participant Manager as LobbyNotificationManager
participant PublicLobby as PublicLobby
participant Audio as AudioContext
User->>Button: click bell
Button->>Window: dispatch "open-notification-modal"
Window->>Modal: open()
Modal->>User: show UI (criteria, sound)
User->>Modal: save settings
Modal->>Window: dispatch "notification-settings-changed"
Window->>Manager: reload settings
PublicLobby->>PublicLobby: fetch lobbies
PublicLobby->>Window: dispatch "lobbies-updated" (lobby list)
Window->>Manager: receive "lobbies-updated"
Manager->>Manager: evaluate lobbies vs criteria, filter seen
alt match && not seen
Manager->>Manager: mark seen
opt sound enabled
Manager->>Audio: create/play beep
Audio->>User: play sound
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/Main.ts (1)
1-1: Fix Prettier formatting.The pipeline detected Prettier formatting issues. Please run
prettier --writeto fix formatting across affected files.#!/bin/bash # Run prettier to fix formatting issues prettier --write "src/client/Main.ts" "src/server/GameManager.ts" "src/server/Worker.ts" "src/client/styles.css" "src/client/LobbyNotificationModal.ts"src/core/Schemas.ts (1)
1-642: Run Prettier to fix formatting.The CI pipeline detected formatting issues. Please run
prettier --write src/core/Schemas.tsto fix them.
🧹 Nitpick comments (4)
src/server/Worker.ts (1)
64-65: Consider periodic cleanup for stale connections.The NotificationBroadcaster has a cleanup() method to remove closed connections, but it's never called. Consider calling it periodically (e.g., every 5 minutes) to ensure stale connections are removed even if the close event isn't properly fired.
Suggested periodic cleanup
const gm = new GameManager(config, log); const notificationBroadcaster = new NotificationBroadcaster(); gm.setNotificationBroadcaster(notificationBroadcaster); + + // Periodic cleanup of stale connections + setInterval(() => { + notificationBroadcaster.cleanup(); + }, 5 * 60 * 1000); // Every 5 minutessrc/client/LobbyNotificationButton.ts (1)
20-26: Consider internationalization for the title.The title attribute "Lobby Notifications" should be passed through the translation system for i18n support, similar to other buttons in the codebase. The bell emoji is a good universal icon and doesn't require translation.
src/client/styles.css (1)
660-707: Consider respecting reduced motion preferences.The notification includes a pulsing animation that runs continuously. Consider respecting the user's motion preferences for accessibility.
Suggested accessibility improvement
.lobby-notification { /* existing styles */ animation: notificationPulse 2s infinite; } +@media (prefers-reduced-motion: reduce) { + .lobby-notification { + animation: none; + } +}src/client/LobbyNotificationManager.ts (1)
84-122: Add exponential backoff for WebSocket reconnection.The reconnection logic uses a fixed 5-second delay. For better resilience, implement exponential backoff to avoid hammering the server during outages.
🔎 Proposed fix with exponential backoff
Add a reconnect attempt counter and use exponential backoff:
+ private reconnectAttempts = 0; + private readonly MAX_RECONNECT_DELAY = 30000; // 30 seconds max + private connectWebSocket() { try { const protocol = window.location.protocol === "https:" ? "wss:" : "ws:"; // Connect to worker 0 for now - TODO: connect to all workers for comprehensive notifications const wsUrl = `${protocol}//${window.location.host}/w0/`; this.ws = new WebSocket(wsUrl); this.ws.onopen = () => { console.log("Notification WebSocket connected"); + this.reconnectAttempts = 0; if (this.reconnectTimer) { clearTimeout(this.reconnectTimer); this.reconnectTimer = null; } // Register if we're on lobby page and have settings if (this.isOnLobbyPage && this.settings) { this.registerPreferences(); } }; // ... onmessage handler ... this.ws.onclose = () => { console.log("Notification WebSocket disconnected, reconnecting..."); this.ws = null; - // Reconnect after 5 seconds + // Exponential backoff: 1s, 2s, 4s, 8s, 16s, up to 30s + const delay = Math.min( + 1000 * Math.pow(2, this.reconnectAttempts), + this.MAX_RECONNECT_DELAY + ); + this.reconnectAttempts++; this.reconnectTimer = window.setTimeout(() => { this.connectWebSocket(); - }, 5000); + }, delay); };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/client/LobbyNotificationButton.ts(1 hunks)src/client/LobbyNotificationManager.ts(1 hunks)src/client/LobbyNotificationModal.ts(1 hunks)src/client/Main.ts(3 hunks)src/client/index.html(1 hunks)src/client/styles.css(1 hunks)src/core/Schemas.ts(5 hunks)src/server/GameManager.ts(3 hunks)src/server/NotificationBroadcaster.ts(1 hunks)src/server/Worker.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/Main.tssrc/client/styles.csssrc/client/LobbyNotificationModal.tssrc/client/LobbyNotificationManager.tssrc/client/index.htmlsrc/client/LobbyNotificationButton.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/LobbyNotificationModal.tssrc/client/LobbyNotificationManager.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/core/Schemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
🧬 Code graph analysis (6)
src/server/GameManager.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/server/NotificationBroadcaster.ts (1)
NotificationBroadcaster(21-128)
src/client/Main.ts (1)
src/client/LobbyNotificationManager.ts (1)
LobbyNotificationManager(15-305)
src/client/LobbyNotificationModal.ts (1)
src/client/LobbyNotificationButton.ts (1)
customElement(4-29)
src/server/NotificationBroadcaster.ts (4)
src/core/game/GameView.ts (1)
gameID(785-787)src/core/Schemas.ts (2)
GameConfig(84-84)ServerLobbyNotificationMessage(112-112)src/core/Util.ts (1)
replacer(330-332)src/core/configuration/DefaultConfig.ts (1)
playerTeams(318-320)
src/client/LobbyNotificationManager.ts (2)
src/core/Schemas.ts (2)
ServerLobbyNotificationMessage(112-112)GameConfig(84-84)src/core/configuration/DefaultConfig.ts (1)
playerTeams(318-320)
src/server/Worker.ts (1)
src/server/NotificationBroadcaster.ts (1)
NotificationBroadcaster(21-128)
🪛 GitHub Actions: 🧪 CI
src/server/GameManager.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/client/Main.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/client/styles.css
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/client/LobbyNotificationModal.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/server/NotificationBroadcaster.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/client/LobbyNotificationManager.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/core/Schemas.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/server/Worker.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
🪛 GitHub Check: 🔍 ESLint
src/server/NotificationBroadcaster.ts
[failure] 16-16:
'RegisteredClient' is defined but never used.
src/client/LobbyNotificationManager.ts
[failure] 184-184:
'gameID' is assigned a value but never used.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (25)
src/client/index.html (1)
464-465: LGTM! Clean DOM integration.The new notification elements are correctly placed in the DOM hierarchy and follow the existing pattern for custom elements in the codebase.
src/client/Main.ts (2)
30-34: LGTM! Clean imports.The new imports follow the existing pattern and are properly organized.
106-108: LGTM! Well-integrated notification system.The notification components are properly initialized with appropriate null checks, and the event wiring follows the existing patterns in the codebase. The optional chaining on Line 181 correctly handles the case where the modal element might not be found.
Also applies to: 164-182
src/server/GameManager.ts (2)
14-29: LGTM! Clean dependency injection.The NotificationBroadcaster integration uses dependency injection via the public setter, which is a clean pattern for optional features. The null-safe field initialization is appropriate.
62-96: LGTM! Improved config handling with safe broadcasting.The refactor to explicitly construct the config object improves readability and makes the defaults clear. The broadcasting is correctly gated behind both a null check and a game-type check (handled in the broadcaster), ensuring it only triggers when appropriate.
src/server/Worker.ts (3)
64-65: LGTM! Clean broadcaster initialization.The NotificationBroadcaster is properly instantiated and wired into the GameManager using the dependency injection setter.
325-328: LGTM! Clean registration handling.The notification preference registration is handled early in the message processing flow, which is appropriate since it doesn't require authentication. The return statement correctly prevents further processing.
491-491: LGTM! Proper cleanup on disconnect.Unregistering the client when the WebSocket closes prevents memory leaks and ensures stale connections don't receive notifications.
src/client/LobbyNotificationButton.ts (1)
1-29: LGTM! Simple and effective button component.The implementation is clean and follows Lit conventions. The createRenderRoot override bypasses shadow DOM, which is consistent with the codebase pattern. The CustomEvent is properly configured to bubble and compose through the DOM.
src/client/styles.css (1)
660-707: LGTM! Well-designed notification styles.The styling is polished with:
- Fixed positioning at the top-center with high z-index appropriate for notifications
- Smooth transitions and a subtle pulsing animation
- Clear hover feedback
- Click-to-dismiss cursor indication
The implementation is visually appealing and follows modern UI patterns.
src/client/LobbyNotificationModal.ts (5)
4-29: LGTM! Clean component setup.The interface and component structure are well-organized. The createRenderRoot override is consistent with the codebase pattern for Tailwind styling.
31-89: LGTM! Robust settings persistence.The lifecycle methods properly set up and tear down event listeners. The localStorage integration includes error handling and dispatches a custom event to notify other components of changes, which is a clean integration pattern.
91-128: LGTM! Well-defined public API.The public methods provide a clean interface for the modal:
getCriteria()returns properly structured criteriaisEnabled()andisSoundEnabled()provide clear state checksopen()andclose()delegate to the underlying modal element
215-248: LGTM! Smart slider constraints.The slider input handlers automatically adjust min/max values to prevent invalid ranges (e.g., min > max). This is good defensive programming that ensures the UI state remains valid without requiring complex validation.
338-370: LGTM! Clear distinction between fixed and variable team modes.The UI clearly separates "Fixed Modes" (Duos, Trios, Quads) from "Variable Modes" (number of teams), making it easy for users to understand the different team configurations.
src/server/NotificationBroadcaster.ts (4)
25-37: LGTM! Clean registration API.The registration method handles both registration (with preferences) and unregistration (with null) in a single method, which is a clean API design. The unregisterClient method provides an explicit alternative for clarity.
39-68: LGTM! Safe broadcasting with error handling.The method correctly:
- Filters to only public games
- Checks for game capacity availability
- Verifies WebSocket is open before sending
- Handles send errors per-client without crashing
70-118: LGTM! Comprehensive preference matching.The preference matching logic correctly handles:
- FFA mode with min/max player constraints
- Team mode with min/max player constraints
- Team configuration filtering based on selected team counts
- Early returns for invalid configurations
121-127: LGTM! Cleanup prevents connection leaks.The cleanup method removes closed connections from the map, preventing memory leaks. As suggested in the Worker.ts review, this should be called periodically.
src/client/LobbyNotificationManager.ts (3)
23-27: LGTM! Clean constructor initialization.The constructor properly initializes settings, event listeners, and WebSocket connection in the correct order.
268-288: LGTM! Clear game details formatting.The text generation logic handles all game modes and team configurations cleanly. The fallback to "Game Found!" is a safe default.
290-304: LGTM! Proper cleanup in destroy method.The destroy method properly closes the WebSocket, clears timers, removes event listeners, and dismisses notifications. Good lifecycle management.
src/core/Schemas.ts (3)
496-502: LGTM! Well-defined notification message schema.The ServerLobbyNotificationMessageSchema properly defines the structure with an optional gameConfig field, allowing the server to send notifications with or without full game details.
565-576: LGTM! Flexible preferences schema with nullable pattern.The schema design is solid:
- Uses nullable preferences to support unregistering (set to null)
- Makes player count filters optional for flexibility
- Accepts both string and number types for team counts
93-94: LGTM! Discriminated unions updated correctly.The new message types are properly added to:
- ClientMessage and ServerMessage type unions
- Type exports
- Discriminated union schemas (ClientMessageSchema, ServerMessageSchema)
This ensures the new notification messages integrate cleanly with the existing message system.
Also applies to: 101-102, 112-112, 120-120, 511-511, 586-586
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/LobbyNotificationManager.ts (1)
202-226: Reuse AudioContext to avoid resource leaks.Creating a new AudioContext on every beep can cause resource leaks in browsers with AudioContext limits. This issue was previously flagged in an earlier review.
🧹 Nitpick comments (2)
src/client/LobbyNotificationManager.ts (1)
274-294: Consider extracting team mode constants to reduce duplication.The team mode strings ("Duos", "Trios", "Quads") are hardcoded here and also appear in
LobbyNotificationModal.ts(lines 164-166, 355). Extracting these to a shared constant would improve maintainability if team modes are added or modified in the future.Example: Shared constants file
Create a new file
src/client/NotificationConstants.ts:export const FIXED_TEAM_MODES = ["Duos", "Trios", "Quads"] as const; export type FixedTeamMode = typeof FIXED_TEAM_MODES[number]; export const VARIABLE_TEAM_COUNTS = [2, 3, 4, 5, 6, 7] as const; export type VariableTeamCount = typeof VARIABLE_TEAM_COUNTS[number];Then import and use in both files to ensure consistency.
src/client/LobbyNotificationModal.ts (1)
162-173: Extract hardcoded team options to shared constants.The team mode values hardcoded here ("Duos", "Trios", "Quads", and numbers 2-7) are duplicated in the render method (lines 355-370, 375-390) and in
LobbyNotificationManager.ts(lines 282-287). Extracting these to a shared constant would prevent inconsistencies if team configurations change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/client/LobbyNotificationManager.ts(1 hunks)src/client/LobbyNotificationModal.ts(1 hunks)src/client/Main.ts(3 hunks)src/client/styles.css(1 hunks)src/core/Schemas.ts(5 hunks)src/server/GameManager.ts(3 hunks)src/server/NotificationBroadcaster.ts(1 hunks)src/server/Worker.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/Main.ts
- src/client/styles.css
- src/server/NotificationBroadcaster.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
📚 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/LobbyNotificationManager.tssrc/client/LobbyNotificationModal.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/LobbyNotificationManager.tssrc/client/LobbyNotificationModal.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/LobbyNotificationManager.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/LobbyNotificationManager.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/LobbyNotificationManager.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/core/Schemas.tssrc/client/LobbyNotificationModal.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
🧬 Code graph analysis (4)
src/server/Worker.ts (1)
src/server/NotificationBroadcaster.ts (1)
NotificationBroadcaster(16-126)
src/client/LobbyNotificationManager.ts (2)
src/core/Schemas.ts (2)
ServerLobbyNotificationMessage(112-114)GameConfig(84-84)src/core/configuration/DefaultConfig.ts (1)
playerTeams(318-320)
src/client/LobbyNotificationModal.ts (1)
src/client/LobbyNotificationButton.ts (1)
customElement(4-29)
src/server/GameManager.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/server/NotificationBroadcaster.ts (1)
NotificationBroadcaster(16-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (11)
src/client/LobbyNotificationManager.ts (2)
15-27: Well-structured notification manager with clean separation of concerns.The class initialization properly sets up event listeners, loads persisted settings, and establishes the WebSocket connection. The architecture cleanly separates concerns between settings management, connection handling, and notification display.
84-132: Solid WebSocket connection handling with automatic reconnection.The WebSocket setup includes proper lifecycle management, error handling, and automatic reconnection after 5 seconds on disconnect. The connection logic correctly registers preferences only when on the lobby page and settings are available.
src/client/LobbyNotificationModal.ts (3)
11-40: Clean component structure with proper lifecycle management.The LitElement component is well-organized with proper state management using decorators, keyboard event handling, and cleanup in
disconnectedCallback. The lifecycle methods correctly add and remove event listeners to prevent memory leaks.
49-92: Solid settings persistence with error handling.The
loadSettings()andsaveSettings()methods properly handle localStorage operations with try/catch blocks and provide sensible defaults. Dispatching a custom event to notify other components of setting changes is a clean approach to coordinating state across the application.
218-248: Slider interaction correctly enforces min ≤ max constraint.The FFA min/max slider handlers properly maintain the invariant that
ffaMinPlayers <= ffaMaxPlayersby adjusting the max when the min exceeds it. This prevents invalid range configurations and provides good UX.src/core/Schemas.ts (1)
500-506: Well-defined schemas for lobby notification messages.The new
ServerLobbyNotificationMessageSchemaandClientRegisterNotificationPreferencesSchemaare properly structured with appropriate field types and validation. Makingpreferencesnullable in the client schema cleanly supports both registration and unregistration flows. The schemas are correctly integrated into the discriminated unions (lines 515, 592) for type-safe message handling.Also applies to: 569-582
src/server/GameManager.ts (2)
27-29: Clean dependency injection pattern for the broadcaster.Using a setter method to inject the
NotificationBroadcasterkeeps the constructor simple and allows the broadcaster to be wired in from the Worker initialization. The nullable type properly reflects that broadcasting is optional.
62-94: Well-structured game creation with centralized config defaults.Constructing the config object with explicit defaults before merging with
gameConfigimproves clarity and consistency. Broadcasting the game creation notification after successful instantiation ensures the broadcaster receives accurate configuration, and the null check prevents errors when no broadcaster is configured.src/server/Worker.ts (3)
64-65: Proper broadcaster initialization and wiring.Instantiating the
NotificationBroadcasteronce and setting it on theGameManagerensures all games created by this worker can broadcast notifications to registered clients. This centralized approach is cleaner than passing the broadcaster to each game individually.
325-328: Correct placement of notification registration handling.Handling the
register_notificationsmessage beforejoin/rejoinchecks is correct, as users should be able to set notification preferences without being in a game. The message is validated by the schema parser at line 305, so the type is safe here.
491-491: Proper cleanup prevents memory leaks.Unregistering the client from the broadcaster in the
closeevent handler ensures that closed WebSocket connections don't accumulate in the broadcaster's registry. This prevents memory leaks and avoids attempting to send notifications to closed connections.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/server/NotificationBroadcaster.ts (2)
6-14: Consider exportingNotificationPreferencesfor type safety.The
NotificationPreferencesinterface is used by both this class and likely byWorker.tswhen handlingregister_notificationsmessages. Exporting it would allow type-safe message handling elsewhere.Proposed change
-interface NotificationPreferences { +export interface NotificationPreferences { ffaEnabled: boolean; teamEnabled: boolean; ffaMinPlayers?: number; ffaMaxPlayers?: number; teamMinPlayers?: number; teamMaxPlayers?: number; selectedTeamCounts?: Array<string | number>; }
102-110: Clarify behavior when no team counts are selected.When
selectedTeamCountsis empty or undefined, this check is skipped and all Team games match. If this is intentional (match any team configuration when none specified), a brief comment would help future readers.Add clarifying comment
// Check team configuration + // If no specific team counts selected, match all team configurations if (prefs.selectedTeamCounts && prefs.selectedTeamCounts.length > 0) { const playerTeams = config.playerTeams;src/client/LobbyNotificationModal.ts (2)
42-47: Consider scoping Escape key to when modal is open.Currently, pressing Escape anywhere on the page calls
close(). While harmless, it could be more precise by checking if the modal is actually open first.Proposed change
private handleKeyDown = (e: KeyboardEvent) => { - if (e.code === "Escape") { + if (e.code === "Escape" && this.modalEl) { e.preventDefault(); this.close(); } };
186-190: Reminder: Add i18n for UI strings.Per the PR checklist, displayed text should be passed through
translateText()and added toen.json. ThetranslationKey=""is empty and strings like "Free For All", "Team Mode", etc. are hardcoded.Would you like me to help identify all the strings that need translation keys?
src/client/LobbyNotificationManager.ts (2)
218-243: AudioContext may needresume()for browser autoplay policy.Browsers may suspend AudioContext until user interaction. If the context is suspended,
oscillator.start()won't produce sound. Consider resuming the context before playing.Proposed change
private playBeepSound() { try { const audioContext = this.getAudioContext(); if (!audioContext) return; + // Resume if suspended (browser autoplay policy) + if (audioContext.state === "suspended") { + audioContext.resume(); + } + const oscillator = audioContext.createOscillator();
292-292: Remove unusedgameCapacityvariable.The
gameCapacityis assigned but never used in the returned strings.Proposed change
private getGameDetailsText(gameConfig: GameConfig): string { - const gameCapacity = gameConfig.maxPlayers ?? null; - if (gameConfig.gameMode === GameMode.FFA) { return `FFA Game Found!`; } else if (gameConfig.gameMode === GameMode.Team) { const playerTeams = gameConfig.playerTeams; if (playerTeams === "Duos") { return `Duos Game Found!`; } else if (playerTeams === "Trios") { return `Trios Game Found!`; } else if (playerTeams === "Quads") { return `Quads Game Found!`; - } else if (typeof playerTeams === "number" && gameCapacity !== null) { + } else if (typeof playerTeams === "number") { return `${playerTeams} Teams Game Found!`; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/client/LobbyNotificationManager.ts(1 hunks)src/client/LobbyNotificationModal.ts(1 hunks)src/client/Main.ts(3 hunks)src/client/styles.css(1 hunks)src/core/Schemas.ts(5 hunks)src/server/GameManager.ts(3 hunks)src/server/NotificationBroadcaster.ts(1 hunks)src/server/Worker.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/Schemas.ts
- src/server/GameManager.ts
- src/server/Worker.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/styles.csssrc/client/LobbyNotificationModal.tssrc/client/LobbyNotificationManager.tssrc/client/Main.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/LobbyNotificationModal.tssrc/client/LobbyNotificationManager.tssrc/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/LobbyNotificationManager.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/LobbyNotificationManager.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/LobbyNotificationManager.ts
🧬 Code graph analysis (3)
src/server/NotificationBroadcaster.ts (4)
src/core/game/GameView.ts (1)
gameID(785-787)src/core/Schemas.ts (2)
GameConfig(84-84)ServerLobbyNotificationMessage(112-114)src/core/Util.ts (1)
replacer(330-332)src/core/configuration/DefaultConfig.ts (1)
playerTeams(318-320)
src/client/LobbyNotificationManager.ts (2)
src/core/Schemas.ts (2)
ServerLobbyNotificationMessage(112-114)GameConfig(84-84)src/core/configuration/DefaultConfig.ts (1)
playerTeams(318-320)
src/client/Main.ts (1)
src/client/LobbyNotificationManager.ts (1)
LobbyNotificationManager(15-332)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (16)
src/server/NotificationBroadcaster.ts (2)
20-32: LGTM!The registration logic is clear:
nullpreferences unregisters, otherwise it registers or updates. Having bothregisterClient(ws, null)andunregisterClient(ws)gives flexibility.
34-66: LGTM!Good defensive coding: filters for Public games, checks capacity, handles send errors per-client so one failure does not block others. The
replacerusage handles BigInt serialization correctly.src/client/Main.ts (3)
30-34: LGTM!Import pattern is consistent with existing imports in this file (side-effect import followed by named class import).
106-108: LGTM!Field declarations follow the existing pattern in this class.
180-182: LGTM!Good wiring: the button dispatches
open-notification-modaland Main.ts listens to open the modal. This decouples the button from directly knowing about the modal.src/client/styles.css (2)
688-718: LGTM!Clean CSS with smooth transitions. The 0.3s transition matches the 300ms timeout in
dismissNotification()for proper animation timing.
660-686: Z-index is safely above all other elements - no conflicts found.The notification uses
z-index: 20000, which is much higher than any other element in the codebase. The next highest z-index is 9999 (setting components), creating a clear separation. No conflicts with error modals or overlays.src/client/LobbyNotificationModal.ts (4)
4-9: LGTM!Clean typed union for
gameModeinstead of class hierarchy. The interface is well-defined with optional fields.
27-29: LGTM!Returning
thisfromcreateRenderRoot()disables Shadow DOM, allowing Tailwind utility classes to work. This is consistent with other Lit components in the codebase.
49-92: LGTM!Good error handling around localStorage. Settings defaults are sensible and the event dispatch notifies the manager of changes.
162-176: LGTM!The select/deselect all helpers are clean and correctly trigger
requestUpdate()andsaveSettings().src/client/LobbyNotificationManager.ts (5)
4-13: LGTM!The
NotificationSettingsinterface matches what the modal saves and dispatches. All fields are typed correctly.
24-28: LGTM!Clean initialization order: load settings, setup listeners, then connect WebSocket. The constructor keeps things simple.
46-62: LGTM!Good state machine: tracks lobby page presence and registers/unregisters preferences on transitions. The path/hash checks cover the main lobby URL patterns.
88-89: TODO: Single worker connection limitation.The comment notes connecting to all workers for comprehensive notifications. Currently, games created on other workers won't trigger notifications. Consider tracking this in a follow-up issue.
Is this a known limitation for the initial implementation, or should it be addressed before merge?
245-289: LGTM!Clean notification lifecycle: create, animate in, click or timeout to dismiss, animate out, remove. The timing (10ms trigger, 10s auto-dismiss, 300ms removal) aligns with CSS transitions.
|
Can you expand on your description just a smidge please |
im still in draft |
My mistake! Didn't see it wasn't open! |
|
I think If many players are waiting for a specific game type, its chance of appearing should increase Your pr handles something different, but I just want to share my thoughts |
Agree, I was going to potentially work on another PR which shows how many people are tracking what if that makes sense |
github-project-automation
bot
moved this to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/client/LobbyNotificationManager.test.ts (3)
71-104: Initialization tests only check existence, not behavior.These three tests only assert that the manager instance is defined, but don't verify:
- Line 72-76: That the manager correctly handles empty settings (e.g., no notification triggered without settings)
- Line 78-94: That settings were actually loaded from localStorage (e.g., dispatch a matching lobby and verify AudioContext is called)
- Line 96-103: That corrupted data is handled gracefully (e.g., no exception thrown, manager continues with null settings)
While the functionality is validated indirectly by later tests, these initialization tests could be strengthened by verifying the expected behavior after each initialization scenario.
🔎 Example: Verify settings were loaded from localStorage
test("should load settings from localStorage on initialization", () => { const settings = { ffaEnabled: true, teamEnabled: false, soundEnabled: true, minTeamCount: 2, maxTeamCount: 4, }; localStorage.setItem( "lobbyNotificationSettings", JSON.stringify(settings), ); const newManager = new LobbyNotificationManager(); - expect(newManager).toBeDefined(); + + // Verify settings were applied by dispatching a matching FFA lobby + const gameConfig: GameConfig = { + gameMap: GameMapType.World, + difficulty: Difficulty.Hard, + donateGold: false, + donateTroops: false, + gameType: GameType.Private, + gameMode: GameMode.FFA, + gameMapSize: GameMapSize.Compact, + disableNations: false, + bots: 0, + infiniteGold: false, + infiniteTroops: false, + instantBuild: false, + randomSpawn: false, + maxPlayers: 10, + }; + + jest.clearAllMocks(); + const event = new CustomEvent("lobbies-updated", { + detail: [{ gameID: "test", gameConfig, numClients: 5 }], + }); + window.dispatchEvent(event); + + // Should trigger notification since FFA is enabled in loaded settings + expect((window as any).AudioContext).toHaveBeenCalled(); newManager.destroy(); });
151-169: Test doesn't validate manager behavior.This test directly manipulates localStorage and verifies the API works, but never interacts with the manager (beyond the one created in
beforeEach). It doesn't verify that the manager actually persists settings whennotification-settings-changedis dispatched.Consider testing the manager's persistence behavior by dispatching a settings event, destroying the manager, creating a new instance, and verifying the new instance loaded the persisted settings.
🔎 Example: Test manager's persistence behavior
test("should persist settings to localStorage", () => { const settings = { ffaEnabled: true, teamEnabled: false, soundEnabled: true, minTeamCount: 2, maxTeamCount: 4, }; - localStorage.setItem( - "lobbyNotificationSettings", - JSON.stringify(settings), - ); - const stored = localStorage.getItem("lobbyNotificationSettings"); - const parsed = JSON.parse(stored ?? "{}"); - - expect(parsed.minTeamCount).toBe(2); - expect(parsed.ffaEnabled).toBe(true); + // Dispatch settings event + const event = new CustomEvent("notification-settings-changed", { + detail: settings, + }); + window.dispatchEvent(event); + + // Note: Current implementation doesn't persist on event + // This test would verify if persistence logic is added + const stored = localStorage.getItem("lobbyNotificationSettings"); + expect(stored).not.toBeNull(); + + if (stored) { + const parsed = JSON.parse(stored); + expect(parsed.ffaEnabled).toBe(true); + expect(parsed.minTeamCount).toBe(2); + } });Note: The current implementation loads from localStorage but doesn't save when settings change via the event. If auto-persistence is desired, the manager would need to save to localStorage in
handleSettingsChanged.
283-321: Inconsistent and verbose assertion pattern.These Team mode tests use a more complex assertion pattern:
expect((window as any).AudioContext.mock.calls.length).toBeGreaterThanOrEqual(1); const audioContextInstance = (window as any).AudioContext.mock.results[0].value; expect(audioContextInstance.createOscillator).toHaveBeenCalled();This differs from the simpler pattern used in FFA tests:
expect((window as any).AudioContext).toHaveBeenCalled();The complex pattern has two issues:
toBeGreaterThanOrEqual(1)is weaker thantoHaveBeenCalled()ortoBe(1)- it allows multiple calls when testing single-notification scenarios- Checking both
AudioContextconstructor ANDcreateOscillatoris redundant - checking one is sufficientConsider using the simpler pattern consistently across all tests for better readability.
🔎 Example: Simplify Duos test assertions
window.dispatchEvent(event); -expect( - (window as any).AudioContext.mock.calls.length, -).toBeGreaterThanOrEqual(1); -const audioContextInstance = (window as any).AudioContext.mock.results[0] - .value; -expect(audioContextInstance.createOscillator).toHaveBeenCalled(); +expect((window as any).AudioContext).toHaveBeenCalled();Also applies to: 323-361, 363-401, 499-537
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/Main.tssrc/client/PublicLobby.tssrc/client/index.htmltests/client/LobbyNotificationManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- resources/lang/en.json
- src/client/PublicLobby.ts
- src/client/index.html
- src/client/Main.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
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.
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
tests/client/LobbyNotificationManager.test.ts
📚 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:
tests/client/LobbyNotificationManager.test.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
tests/client/LobbyNotificationManager.test.ts
🧬 Code graph analysis (1)
tests/client/LobbyNotificationManager.test.ts (3)
src/client/LobbyNotificationManager.ts (1)
LobbyNotificationManager(12-176)src/core/Schemas.ts (2)
GameConfig(86-86)GameInfo(130-136)src/server/GameServer.ts (1)
gameInfo(699-711)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (2)
tests/client/LobbyNotificationManager.test.ts (2)
917-987: LGTM! Thorough cleanup testing.These tests properly verify that the manager cleans up resources:
- Event listeners are removed using
removeEventListener- AudioContext is closed when it exists
The tests use appropriate patterns (spy for listener removal, mock verification for AudioContext closure) to validate cleanup behavior.
989-1100: LGTM! Comprehensive edge case coverage.These tests cover important edge cases and error conditions:
- Undefined or null event details
- Undefined gameConfig
- Missing settings (manager should not crash)
- Missing optional fields like maxPlayers
All tests verify that the manager handles these scenarios gracefully without throwing exceptions. This defensive coding approach is good for robustness.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/client/LobbyNotificationManager.test.ts (3)
272-285: Same duplicate manager issue here.Like the FFA section, this nested
beforeEachreassignsmanagerwithout destroying the previous instance. Apply the same fix: callmanager.destroy()before creating the new one.
725-738: Same duplicate manager issue here.Like previous sections, this nested
beforeEachshould destroy the existing manager before creating a new one.
974-987: Same duplicate manager issue here.Like previous sections, this nested
beforeEachshould destroy the existing manager before creating a new one.
🧹 Nitpick comments (2)
tests/client/LobbyNotificationManager.test.ts (2)
155-173: Test name does not match what is tested.This test manually writes to and reads from
localStorage. It does not verify thatLobbyNotificationManagerpersists settings. The manager'shandleSettingsChangedonly updates the in-memorysettingsfield without saving to localStorage (based on the snippet provided).Consider either:
- Rename to clarify it tests localStorage API behavior, or
- Update to test actual manager persistence if that feature exists
176-190: Potential duplicate manager instances.The outer
beforeEach(line 68) creates a manager assigned tomanager. This nestedbeforeEach(line 189) creates another manager and reassignsmanager. The first manager is not destroyed, leaving its event listeners active until the test ends.This could cause false positives if both managers react to the same event. Consider calling
manager.destroy()before reassigning:🔎 Proposed fix
beforeEach(() => { const settings = { ffaEnabled: true, teamEnabled: false, soundEnabled: true, minTeamCount: 2, maxTeamCount: 4, }; localStorage.setItem( "lobbyNotificationSettings", JSON.stringify(settings), ); + manager.destroy(); manager = new LobbyNotificationManager(); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
index.htmlresources/lang/en.jsonsrc/client/LangSelector.tssrc/client/Main.tssrc/client/PublicLobby.tstests/client/LobbyNotificationManager.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/lang/en.json
- src/client/LangSelector.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
📚 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/PublicLobby.tstests/client/LobbyNotificationManager.test.tssrc/client/Main.tsindex.html
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
tests/client/LobbyNotificationManager.test.tssrc/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
tests/client/LobbyNotificationManager.test.tssrc/client/Main.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (2)
tests/client/LobbyNotificationManager.test.ts (2)
src/client/LobbyNotificationManager.ts (1)
LobbyNotificationManager(12-176)src/core/Schemas.ts (2)
GameConfig(86-86)GameInfo(130-136)
src/client/Main.ts (1)
src/client/LobbyNotificationManager.ts (1)
LobbyNotificationManager(12-176)
🔇 Additional comments (13)
src/client/PublicLobby.ts (1)
57-63: Clean event-based communication.Using a global
CustomEventonwindowis a simple way to decouple PublicLobby from the notification manager. The implementation is straightforward and matches howLobbyNotificationManager.setupEventListeners()subscribes to"lobbies-updated".tests/client/LobbyNotificationManager.test.ts (8)
20-69: Good test setup with proper mocks.The
beforeEachblock correctly mockslocalStorageandAudioContext. The mock structure matches whatLobbyNotificationManagerexpects (oscillator, gain node, timing). Cleanup inafterEachensures no leaks between tests.
75-108: Initialization tests cover key scenarios.Testing empty, valid, and corrupted localStorage states ensures the manager starts gracefully in all cases. Since
settingsis private, checkingtoBeDefined()is acceptable here; behavior-based verification is done in later tests.
192-268: FFA matching tests have proper assertions.Both tests now verify actual behavior by checking
AudioContextcalls. The "should not match when FFA is disabled" test correctly clears mocks before dispatching the lobby event and assertsnot.toHaveBeenCalled().
271-575: Team matching tests are thorough.Good coverage of string-based modes (Duos, Trios, Quads) and numeric team calculations. Boundary tests for
minTeamCountandmaxTeamCountverify the matching logic correctly rejects out-of-range configurations.
577-722: Sound notification tests cover key scenarios.Testing enabled, disabled, and error states ensures robust sound handling. The error test at lines 671-721 verifies the manager does not crash when
AudioContextfails, which is good for browser compatibility.
782-833: Realistic lobby update behavior.The test correctly sends
[gameInfo1, gameInfo2]in the second event (line 826), matching real-world behavior wherelobbies-updatedcontains all current lobbies. This addresses the earlier review feedback.
903-971: Cleanup tests verify proper resource release.Testing both event listener removal and
AudioContext.close()ensures the manager does not leak resources. The spy cleanup at line 917 prevents test pollution.
973-1085: Edge case tests ensure robustness.Testing undefined/null values for
gameConfig, eventdetail, settings, andmaxPlayersconfirms the manager handles malformed data gracefully without throwing.index.html (1)
467-468: Custom elements added correctly.The
lobby-notification-buttonandlobby-notification-modalelements are placed alongside other UI components. No special attributes are needed since configuration is handled via events and localStorage.src/client/Main.ts (3)
30-34: Imports follow existing patterns.Side-effect imports for custom element registration plus type imports for DOM queries. Consistent with how other modals and buttons are imported in this file.
185-204: Initialization follows established patterns.DOM queries with warnings match how other components are set up. The manager is created independently of DOM elements, which is correct since it listens to global events.
538-557: Cleanup method is well-structured.Proper cleanup of event listeners and manager destruction. Setting
lobbyNotificationManagertonullafter destroy prevents accidental reuse. ThegameStopcleanup is a good addition.Note: The
handleBeforeUnloadremoval at line 544 is orphaned since the listener was never added (see earlier comment).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/Main.ts (1)
532-552: Consider optional chaining for consistency.The cleanup logic is correct and properly handles teardown during page unload. The method removes listeners, destroys the manager, and stops any ongoing game.
One minor consistency suggestion: line 541 could use optional chaining like the rest of the codebase:
🔎 Optional refactor for consistency
- if (this.lobbyNotificationManager) { - this.lobbyNotificationManager.destroy(); - this.lobbyNotificationManager = null; - } + this.lobbyNotificationManager?.destroy(); + this.lobbyNotificationManager = null;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/Main.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
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.
📚 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/Main.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (1)
src/client/Main.ts (2)
src/client/LobbyNotificationManager.ts (1)
LobbyNotificationManager(12-176)src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(205-205)
🔇 Additional comments (4)
src/client/Main.ts (4)
30-34: LGTM!The imports and field declarations follow the existing codebase patterns correctly.
Also applies to: 116-119
120-128: LGTM!The event handlers are properly defined using arrow functions to preserve context. The optional chaining on line 122 correctly handles cases where the modal isn't found.
Note: The async
handleBeforeUnloadhandler may not complete before the page unloads, but since the critical cleanup operations (removeEventListener,destroy()) are synchronous, they'll execute immediately. Theawait crazyGamesSDK.gameplayStop()is best-effort.
185-204: LGTM!The initialization logic follows the established pattern for other UI components in this file. The manager is correctly instantiated once and will persist throughout the session, which aligns with the intended design.
215-215: LGTM!The beforeunload event listener is correctly registered, and per the resolved review comments, the previous duplicate listener issue has been fixed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/Main.ts (1)
126-129: Consider synchronous cleanup for beforeunload.The
cleanup()method is async and awaitscrazyGamesSDK.gameplayStop()at line 555. However, browsers impose strict time limits onbeforeunloadhandlers and may not wait for async operations to complete. This could prevent proper cleanup before the page unloads.Additionally, line 543 removes the
beforeunloadlistener from within the handler itself, which is redundant since the handler is already executing its final run.🔎 Recommendations
Make time-critical cleanup synchronous: Move synchronous cleanup (listener removal, destroying manager) to a separate method that can execute immediately in
beforeunload. Keep async operations optional or use them elsewhere.Remove redundant listener removal: Line 543 can be removed since the handler is executing and won't be called again. The listener will be garbage collected with the page anyway.
Example refactor:
private handleBeforeUnload = async () => { console.log("Browser is closing"); - await this.cleanup(); + this.cleanupSync(); }; + private cleanupSync(): void { + // Remove event listeners + window.removeEventListener( + "open-notification-modal", + this.handleOpenNotificationModal, + ); + // No need to remove beforeunload - already executing + + // Destroy the LobbyNotificationManager + if (this.lobbyNotificationManager) { + this.lobbyNotificationManager.destroy(); + this.lobbyNotificationManager = null; + } + + // Stop game synchronously if possible + if (this.gameStop !== null) { + this.gameStop(); + this.gameStop = null; + } + } private async cleanup(): Promise<void> { // Keep this for other cleanup scenarios + this.cleanupSync(); - // Remove event listeners - window.removeEventListener( - "open-notification-modal", - this.handleOpenNotificationModal, - ); - window.removeEventListener("beforeunload", this.handleBeforeUnload); - - // Destroy the LobbyNotificationManager - if (this.lobbyNotificationManager) { - this.lobbyNotificationManager.destroy(); - this.lobbyNotificationManager = null; - } - - // Stop any ongoing game - if (this.gameStop !== null) { - this.gameStop(); - this.gameStop = null; + // Async operations that may not complete in beforeunload + try { await crazyGamesSDK.gameplayStop(); + } catch (error) { + console.error("Failed to stop gameplay:", error); } }Also applies to: 537-557
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
index.htmlresources/lang/en.jsonsrc/client/Main.tssrc/client/PublicLobby.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/lang/en.json
- index.html
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
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.
📚 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/PublicLobby.tssrc/client/Main.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/Main.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/Main.ts
🧬 Code graph analysis (1)
src/client/Main.ts (2)
src/client/LobbyNotificationManager.ts (1)
LobbyNotificationManager(12-176)src/client/CrazyGamesSDK.ts (1)
crazyGamesSDK(205-205)
🔇 Additional comments (4)
src/client/PublicLobby.ts (1)
52-57: LGTM! Event dispatch enables notification system.The "lobbies-updated" event is correctly emitted after updating the lobbies state and before processing individual lobbies. This allows the LobbyNotificationManager to observe lobby changes without tight coupling.
src/client/Main.ts (3)
30-34: LGTM! Clean integration of notification components.The imports and field declarations follow the existing codebase patterns with nullable private fields for the notification UI components and manager.
Also applies to: 117-120
122-124: LGTM! Initialization follows established patterns.The event handlers and initialization properly:
- Use optional chaining for nullable components
- Query DOM elements with appropriate warnings
- Create the manager instance once
- Register the event listener for opening the modal
Also applies to: 186-205
666-678: LGTM! Manager lifecycle correctly preserved.The
handleLeaveLobbymethod correctly does not destroy theLobbyNotificationManager, allowing notifications to persist across lobby sessions. This addresses the concern from previous reviews where destroying the manager on lobby leave would break notifications permanently.
Description:
Adds a lobby notification system.
Inspired by https://github.com/DeLoWaN/openfront-autojoin-lobby
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n