Lobby chat panel #2514
Lobby chat panel #2514
Conversation
|
|
WalkthroughAdds an in-lobby chat: UI component, event and transport wiring, new client/server message schemas, server broadcast and config flag, localization, TypeScript declarations, tests, and a deterministic jose mock for test tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Player
participant Panel as LobbyChatPanel
participant Bus as EventBus
participant Transport
participant Server
participant Others as Other Clients
Note right of Panel `#E8F6FF`: Compose message (≤300 chars)
Player->>Panel: Type message & press Enter / click Send
Panel->>Bus: Dispatch SendLobbyChatEvent(text)
Bus->>Transport: Event routed
Transport->>Server: Send `ClientLobbyChatMessage` (ws/local)
Server->>Server: check phase==Lobby && chatEnabled
alt allowed
Server->>Others: Broadcast `ServerLobbyChatMessage` (username,isHost,text)
else blocked
Server->>Server: Ignore message
end
Others->>ClientGameRunner: Receive `lobby_chat`
ClientGameRunner->>Bus: Dispatch `lobby-chat:message`
Bus->>Panel: Panel appends and renders message
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 (4 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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: 2
🧹 Nitpick comments (7)
src/client/graphics/layers/StructureIconsLayer.ts (1)
275-276: Consider extracting filter constants to reduceas anycasts.The
as anycast is repeated six times. You could define typed constants once and reuse them, which improves readability and keeps the type workaround in one place:// Near top of file or in a shared utils const OUTLINE_RED = new OutlineFilter(2, 0xff0000) as PIXI.Filter; const OUTLINE_GREEN = new OutlineFilter(2, 0x00ff00) as PIXI.Filter; const OUTLINE_WHITE = new OutlineFilter(2, 0xffffff) as PIXI.Filter;Then use
[OUTLINE_RED],[OUTLINE_GREEN], etc. in your filter assignments. This keeps the type cast in one spot and makes the intent clearer.src/client/ClientGameRunner.ts (1)
77-84: Consider alternatives to global window pollution.While exposing
EventBusandclientIDglobally enables lightweight components to access them, this pattern bypasses type safety and creates implicit dependencies.Consider these alternatives:
Option 1: Pass EventBus via CustomEvent detail
- (window as any).__eventBus = eventBus; - (window as any).__clientID = lobbyConfig.clientID; document.dispatchEvent( new CustomEvent("event-bus:ready", { + detail: { eventBus, clientID: lobbyConfig.clientID }, bubbles: true, composed: true }), );Then components can capture the EventBus from the event and store it internally.
Option 2: Create a typed global interface
If globals are necessary, at least make them type-safe:
declare global { interface Window { __eventBus?: EventBus; __clientID?: ClientID; } } window.__eventBus = eventBus; window.__clientID = lobbyConfig.clientID;src/server/GameServer.ts (1)
299-315: Avoid re-parsing the raw message; use the already-validated payloadYou already validated the message with
ClientMessageSchemaand haveclientMsgtyped. Re‑parsingmessageand pulling.textfromanyis unnecessary and slightly weaker.You can simplify and keep things fully type‑safe:
- case "lobby_chat": { - // Validate lobby chat usage: must be in lobby phase and chat enabled - if (this.phase() !== GamePhase.Lobby) { - return; - } - if (!this.gameConfig.chatEnabled || this.isPublic()) { - return; - } - // Broadcast to all clients in the same lobby - const payload = JSON.stringify({ - type: "lobby_chat", - sender: client.clientID, - text: (JSON.parse(message) as any).text, - }); - this.activeClients.forEach((c) => c.ws.send(payload)); - break; - } + case "lobby_chat": { + // Validate lobby chat usage: must be in lobby phase and chat enabled + if (this.phase() !== GamePhase.Lobby) { + return; + } + if (!this.gameConfig.chatEnabled || this.isPublic()) { + return; + } + // Broadcast to all clients in the same lobby + const payload = JSON.stringify({ + type: "lobby_chat", + sender: client.clientID, + text: clientMsg.text, + }); + this.activeClients.forEach((c) => c.ws.send(payload)); + break; + }src/client/HostLobbyModal.ts (1)
55-56: Private-lobby chat toggle wiring looks coherent end-to-endState → checkbox →
putGameConfig→ conditional<lobby-chat-panel>render are all consistent, and usingtranslateText("lobby_chat.*")keeps it in the i18n flow.If you ever reuse this modal for non‑private lobbies, consider also disabling or hiding the checkbox in the UI when the underlying
gameTypeis public, so the client and server rules stay aligned from the player’s point of view. For the current private‑only usage, this is fine as is.Also applies to: 414-435, 591-602, 800-801
src/client/Transport.ts (1)
175-178: Type the lobby_chat client message instead of usingas anyThe event wiring is consistent with other intents, but the
sendMsg(msg as any)weakens type safety and hides future mistakes.Consider exporting a dedicated TS type for lobby chat messages from
src/core/Schemas.ts(and adding it to theClientMessageunion), then using it here, e.g.:// After you export something like `ClientLobbyChatMessage` from Schemas: import { // ... ClientLobbyChatMessage, } from "../core/Schemas"; // ... private onSendLobbyChat(event: SendLobbyChatEvent) { const msg: ClientLobbyChatMessage = { type: "lobby_chat", text: event.text, clientID: this.lobbyConfig.clientID, }; this.sendMsg(msg); }This keeps the transport strongly typed and in sync with the Zod schema.
Also applies to: 264-264, 646-654
src/client/components/LobbyChatPanel.ts (1)
45-50: Wait for Lit’s update before auto‑scrolling to ensure it reaches the bottomRight now you update
messagesand immediately query.messagesto setscrollTop. Because Lit renders asynchronously, the container might not yet include the new DOM when you readscrollHeight, so the scroll can be off.You can make this deterministic with
updateComplete:private onIncoming = (e: CustomEvent<{ sender: string; text: string }>) => { const { sender, text } = e.detail; - this.messages = [...this.messages, { sender, text }]; - const container = this.renderRoot.querySelector(".messages") as HTMLElement; - if (container) container.scrollTop = container.scrollHeight; + this.messages = [...this.messages, { sender, text }]; + this.updateComplete.then(() => { + const container = this.renderRoot.querySelector( + ".messages", + ) as HTMLElement | null; + if (container) { + container.scrollTop = container.scrollHeight; + } + }); };This keeps the scroll behavior stable even under heavier chat traffic.
src/core/Schemas.ts (1)
492-496: Schema structure looks good; consider inlining text definition for clarity.The server lobby chat schema follows the discriminated union pattern correctly. The text field uses
SafeString.max(300), which works but chains a 300-char limit onto SafeString's existing 1000-char limit. Both constraints apply, so the effective limit is 300.For clarity, consider defining the text field inline:
export const ServerLobbyChatSchema = z.object({ type: z.literal("lobby_chat"), sender: ID, - text: SafeString.max(300), + text: z.string() + .regex(/^([a-zA-Z0-9\s.,!?@#$%&*()\-_+=[\]{}|;:"'/\u00a9|\u00ae|[\u2000-\u3300]|\ud83c[\ud000-\udfff]|\ud83d[\ud000-\udfff]|\ud83e[\ud000-\udfff]|[üÜ])*$/u) + .max(300), });Or extract a shared
ChatMessageTextschema if both client and server schemas use the same validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
__mocks__/jose.js(1 hunks)jest.config.ts(1 hunks)package.json(1 hunks)resources/lang/en.json(1 hunks)src/client/ClientGameRunner.ts(2 hunks)src/client/HostLobbyModal.ts(5 hunks)src/client/JoinPrivateLobbyModal.ts(4 hunks)src/client/SinglePlayerModal.ts(1 hunks)src/client/Transport.ts(3 hunks)src/client/components/LobbyChatPanel.ts(1 hunks)src/client/graphics/layers/StructureIconsLayer.ts(4 hunks)src/core/Schemas.ts(6 hunks)src/server/GameManager.ts(1 hunks)src/server/GameServer.ts(2 hunks)src/server/MapPlaylist.ts(1 hunks)tests/LobbyChatPanel.test.ts(1 hunks)tests/LobbyChatSchemas.test.ts(1 hunks)tests/util/Setup.ts(1 hunks)
🧰 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-14T00:56:15.437Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Cosmetics.ts:1-1
Timestamp: 2025-06-14T00:56:15.437Z
Learning: The `import { base64url } from "jose";` syntax works correctly in ESM environments for the jose library, contrary to potential assumptions about needing specific ESM import paths like "jose/util/base64url".
Applied to files:
jest.config.ts__mocks__/jose.js
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 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/SinglePlayerModal.tssrc/client/ClientGameRunner.tssrc/server/GameServer.tssrc/client/JoinPrivateLobbyModal.tssrc/client/components/LobbyChatPanel.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/SinglePlayerModal.tstests/LobbyChatSchemas.test.tssrc/server/GameServer.tssrc/core/Schemas.tssrc/server/MapPlaylist.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/ClientGameRunner.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/ClientGameRunner.tssrc/client/HostLobbyModal.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 (5)
tests/LobbyChatSchemas.test.ts (1)
src/core/Schemas.ts (3)
GameConfigSchema(161-181)ClientLobbyChatSchema(524-528)ServerLobbyChatSchema(492-496)
src/client/Transport.ts (1)
src/core/EventBus.ts (1)
GameEvent(1-1)
src/server/GameServer.ts (2)
src/server/MapPlaylist.ts (1)
gameConfig(78-105)src/core/configuration/DefaultConfig.ts (1)
gameConfig(266-268)
src/client/components/LobbyChatPanel.ts (2)
src/core/EventBus.ts (1)
EventBus(7-44)src/client/Transport.ts (1)
SendLobbyChatEvent(176-178)
src/client/HostLobbyModal.ts (2)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)
🪛 GitHub Actions: 🧪 CI
__mocks__/jose.js
[error] 1-1: ESLint Parsing error: /home/runner/work/OpenFrontIO/OpenFrontIO/mocks/jose.js was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProject.
🔇 Additional comments (18)
src/client/graphics/layers/StructureIconsLayer.ts (2)
505-506: LGTM!The hex color values (0xffffff for white) and new constructor signature are correctly applied for the focused structure highlighting.
1-1: I'll help you verify the review comment about@pixi/filter-outlinepackage compatibility. Let me start by examining the project's dependencies and verifying the package information.
<function_calls>
#!/bin/bashFind package.json to check PixiJS version
find . -name "package.json" -type f | head -5
</function_calls>Now let me check the PixiJS version and examine the package configuration:
<function_calls>
#!/bin/bashRead package.json to find PixiJS version
if [ -f "package.json" ]; then
cat package.json | grep -A 2 -B 2 "pixi"
fi
</function_calls>Let me also verify the current state of the file to understand the import change:
<function_calls>
#!/bin/bashCheck the StructureIconsLayer.ts file
if [ -f "src/client/graphics/layers/StructureIconsLayer.ts" ]; then
head -20 src/client/graphics/layers/StructureIconsLayer.ts
fi
</function_calls>Now let me search the web for information about @pixi/filter-outline compatibility:
<function_calls>
@pixi/filter-outline npm package compatibility pixi.js v8
</function_calls>tests/util/Setup.ts (1)
64-64: LGTM!The addition of
chatEnabled: falsecorrectly disables lobby chat in test configurations by default. Tests can override this via the_gameConfigparameter if they need to test chat functionality.resources/lang/en.json (1)
506-511: LGTM!The lobby chat localization keys are well-structured and follow the existing naming conventions. The English strings are clear and grammatically correct.
src/client/ClientGameRunner.ts (1)
138-147: LGTM!The lobby chat message relay logic is clean and straightforward. Dispatching a custom event allows UI components to listen for chat messages without tight coupling to the transport layer.
src/client/SinglePlayerModal.ts (1)
578-578: LGTM!Setting
chatEnabled: falsefor single-player games is the correct behavior, as lobby chat is only relevant for multiplayer lobbies.src/server/GameManager.ts (1)
54-54: LGTM!Adding
chatEnabled: falseto the default game configuration is appropriate. This ensures chat is disabled by default and must be explicitly enabled, which is a safe default behavior.jest.config.ts (1)
8-8: I'll verify whether the jose mock covers all usage across the codebase. Let me search for all jose imports and check the mock implementation.
#!/bin/bashFind all imports from jose library
echo "=== Jose imports in codebase ==="
rg -n --type=ts --type=tsx 'from ["']jose' -A 1echo ""
echo "=== Jose function calls/usage ==="
rg -n --type=ts --type=tsx '\b(base64url|jwtVerify|decodeJwt|JWK)\b' -C 1echo ""
echo "=== Checking mocks/jose.js content ==="
cat -n mocks/jose.js 2>/dev/null || echo "File not found at mocks/jose.js"echo ""
echo "=== Checking alternative mock paths ==="
find . -name "jose.js" -o -name "jose.ts" 2>/dev/null | head -20
package.json (1)
35-35: The@pixi/filter-outline@5.2.0package version is valid and free from known security vulnerabilities.Web verification confirms that @pixi/filter-outline version 5.2.0 exists on the npm registry (published Feb 28, 2023) and has no reported security vulnerabilities according to Snyk and the npm package database.
src/server/MapPlaylist.ts (1)
78-104: DefaultingchatEnabledto false for public playlist is correctKeeping
chatEnabledexplicitly false in the public playlist config matches the schema default and the server’s public‑lobby enforcement. No change needed.tests/LobbyChatPanel.test.ts (1)
1-35: Good minimal coverage for lobby_chat i18n keysThese lightweight tests are a reasonable stopgap: they ensure the new
lobby_chat.*keys exist and are non‑empty until proper component tests are possible.src/server/GameServer.ts (1)
119-124: Server-side guard for public lobby chat is in the right placeUsing
isPublic()here to forcechatEnabledtofalseensures clients can’t enable chat for public games even if they try to PUT it in the config.src/client/JoinPrivateLobbyModal.ts (1)
8-9: Joiner-side chatEnabled reflection and UI are wired correctlyYou read
data.gameConfig?.chatEnabledon each poll and gate<lobby-chat-panel>on that flag, so joiners only see chat when the host has enabled it on the server. This matches the intended “host controls chat toggle” behavior.Also applies to: 22-23, 78-107, 335-337
tests/LobbyChatSchemas.test.ts (1)
1-63: Schema tests cover the key lobby chat behaviorsThese tests validate the
chatEnableddefault and the main happy/validation paths for client/server lobby chat messages, including the max‑length guard. Good targeted coverage for the new schemas.src/core/Schemas.ts (4)
92-93: LGTM! Type unions extended correctly.The lobby chat message types are properly added to the client and server message unions, following the existing patterns in the codebase.
Also applies to: 100-101, 117-118
170-171: LGTM! Good use of default value for backward compatibility.The
chatEnabledfield with.default(false)ensures existing game configs without this field will parse correctly and default to disabled chat. This is the right approach for adding a new optional feature flag to an established schema.
524-528: Client schema mirrors server schema correctly.The client lobby chat schema properly uses
clientIDto identify the sender, matching the pattern used inBaseIntentSchemafor other client-originated messages. The same optional refactor for the text field definition applies here.
505-505: LGTM! Schemas properly integrated into discriminated unions.Both lobby chat schemas are correctly added to their respective discriminated unions, enabling type-safe runtime parsing of chat messages.
Also applies to: 564-564
- Add maxlength=300 to lobby chat input to prevent typing beyond limit - Truncate messages to 300 chars in sendMessage to prevent paste overflow - Add __mocks__ to tsconfig include to fix ESLint parsing errors - Prevents server SafeString validation errors and WebSocket disconnects
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/client/components/LobbyChatPanel.ts (1)
20-33: Consider typed event handlers instead ofas anycasts.The
as anycasts work but lose type safety. You can use a typed listener approach:connectedCallback(): void { super.connectedCallback(); - document.addEventListener("lobby-chat:message", this.onIncoming as any); + document.addEventListener("lobby-chat:message", this.onIncoming as EventListener); // ... - document.addEventListener("event-bus:ready", this.onBusReady as any); + document.addEventListener("event-bus:ready", this.onBusReady as EventListener); }Or keep the handler signature compatible with
EventListenerby checking the event type inside the handler.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.husky/pre-commit(1 hunks)eslint.config.js(1 hunks)src/client/components/LobbyChatPanel.ts(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
🧰 Additional context used
🧠 Learnings (2)
📓 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/components/LobbyChatPanel.ts
🧬 Code graph analysis (1)
src/client/components/LobbyChatPanel.ts (2)
src/core/EventBus.ts (1)
EventBus(7-44)src/client/Transport.ts (1)
SendLobbyChatEvent(176-178)
🔇 Additional comments (5)
eslint.config.js (1)
28-28: LGTM! Follows the established pattern.The addition of
__mocks__/jose.jstoallowDefaultProjectis consistent with howfileMock.jsis handled on line 27. This ensures ESLint can properly lint the new jose mock file.src/client/components/LobbyChatPanel.ts (2)
1-10: Clean imports and typed interface.Good use of a simple typed interface for
ChatMessage. Keeps things clear.
76-108: Good template structure with proper 300-char limit.Clean render method. The
maxlength="300"on the input matches the server-side limit. Message alignment logic is straightforward..husky/pre-commit (2)
5-6:npx lint-stagedusage looks fineKeeping
npx lint-stagedhere is simple and clear, and matches common Husky setups as long as Node is on PATH.
1-3: Based on my research, the review comment is substantially accurate regarding Husky v10 and git hooks:Verified claims:
- Shebang is recommended: While Husky v10 executes hooks via sh (potentially wrapping execution), a POSIX-sh compatible shebang (
#!/bin/sh) is still a best practice for portability and clarity. This is not strictly required by Husky but is strongly recommended for hooks.- PATH export is a no-op:
export PATH="$PATH"does nothing functionally and the comment above it is misleading.- File must be executable: Husky requires hook files to be executable (
chmod +x).- POSIX-sh compatibility: The hook code should be POSIX-sh compatible (which
npx lint-stagedis).Minor clarification: The review slightly overstates the severity by saying missing shebang causes "exec format error" and hook won't run. With Husky v10, the shebang is less critical because Husky wraps execution. However, adding it is still the right move for robustness and follows industry best practices.
Add a proper shebang and clean up the PATH line
The hook should include
#!/bin/shfor clarity and portability, even though Husky v10 wraps execution. Also,export PATH="$PATH"is a no-op; the comment above it is misleading.I suggest:
+#!/bin/sh -# Husky v10 compatible pre-commit (removed deprecated husky.sh sourcing) -# Ensure PATH includes typical Node locations if needed (optional on most setups) -export PATH="$PATH" +# Husky v10 compatible pre-commit (removed deprecated husky.sh sourcing) +# (Optional) Uncomment if you need to extend PATH, e.g., for custom bins. +# export PATH="$PATH:./node_modules/.bin"Also ensure
.husky/pre-commitis executable (chmod +x .husky/pre-commit).
…mespace CSS classes\n\n- Await updateComplete before auto-scrolling to latest message\n- Warn and keep input when EventBus unavailable; clear only on success\n- Prefix CSS classes with lcp- to avoid global style conflicts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/components/LobbyChatPanel.ts (1)
45-55: Async auto-scroll after DOM update looks correctUsing
await this.updateCompletebefore querying.lcp-messagesand adjustingscrollTopfixes the earlier race where scroll could happen before the new message rendered. The null-check oncontaineris also enough here.
🧹 Nitpick comments (3)
src/client/components/LobbyChatPanel.ts (3)
20-39: Avoidanycasts on event listeners and globalsThe lifecycle wiring looks correct and cleanup is handled, but you can tighten types a bit:
- Replace
as anyonaddEventListener/removeEventListenerwith a typed listener, e.g. useEventListeneror changeonIncomingto acceptEventand cast inside.- Consider defining a small typed helper for reading
(window as any).__eventBusand(window as any).__clientIDso these globals are concentrated and strongly typed in one place.This keeps the component more idiomatic and avoids
anyholes.// Example pattern for onIncoming typing: private onIncoming = async (e: Event) => { const { sender, text } = (e as CustomEvent<{ sender: string; text: string }>).detail; // ... };
65-84: sendMessage flow and failure handling are in good shapeThe flow is nice and simple:
- Trim + early return on empty.
- Lazy attach to global
__eventBus.- Warn and keep the text if the bus is still missing.
- Enforce the 300-char cap before emitting and clear the input only on success.
If you want to reduce duplication later, you could pull the “resolve bus from
window.__eventBus” part into a small helper, but not required for this PR.
125-138: Guard global style injection for non-DOM environmentsThe prefixed CSS classes are good for avoiding conflicts, but the top-level
document.head.appendChild(style)runs at module load. If this file is ever imported in a non-DOM environment (SSR, some tests),documentmay be undefined and blow up on import.You can make this safer with a small guard:
-const style = document.createElement("style"); -style.textContent = ` -... -`; -document.head.appendChild(style); +if (typeof document !== "undefined") { + const style = document.createElement("style"); + style.textContent = ` + ... + `; + document.head.appendChild(style); +}Functionality stays the same in the browser, but imports become safer elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/components/LobbyChatPanel.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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/components/LobbyChatPanel.ts
🧬 Code graph analysis (1)
src/client/components/LobbyChatPanel.ts (2)
src/core/EventBus.ts (1)
EventBus(7-44)src/client/Transport.ts (1)
SendLobbyChatEvent(176-178)
🔇 Additional comments (1)
src/client/components/LobbyChatPanel.ts (1)
1-19: Imports and basic state look solidImports match usage, and the
ChatMessageshape plusmessages/inputTextstate are clear and minimal. No changes needed here.
Add preventDefault() and stopPropagation() in keydown handler to avoid unintended parent handlers and default behaviors while sending chat messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/client/components/LobbyChatPanel.ts (4)
20-33: Consider improving type safety for window properties.The repeated
(window as any).__eventBusand(window as any).__clientIDaccesses lose TypeScript's protection. Consider declaring these properties in a global type definition or creating typed accessors.For example, add to a
global.d.tsfile:declare global { interface Window { __eventBus?: EventBus; __clientID?: string; } }Then use
window.__eventBusdirectly without casting. This provides autocomplete and catches typos at compile time.
45-55: Consider adding a message history limit.The
messagesarray grows unbounded. In a long lobby session, this could accumulate hundreds of messages, impacting memory and render performance.Apply this diff to keep only the most recent messages:
private onIncoming = async ( e: CustomEvent<{ sender: string; text: string }>, ) => { const { sender, text } = e.detail; - this.messages = [...this.messages, { sender, text }]; + const MAX_MESSAGES = 100; + const updated = [...this.messages, { sender, text }]; + this.messages = updated.slice(-MAX_MESSAGES); await this.updateComplete; const container = this.renderRoot.querySelector( ".lcp-messages", ) as HTMLElement | null; if (container) container.scrollTop = container.scrollHeight; };This keeps the last 100 messages, which is sufficient for lobby chat and prevents unbounded growth.
86-122: Consider adding accessibility attributes.The chat panel lacks semantic markup and ARIA labels, which impacts screen reader users.
Apply these accessibility improvements:
render() { return html` <div class="lcp-container"> - <div class="lcp-messages"> + <div class="lcp-messages" role="log" aria-live="polite" aria-label="Lobby chat messages"> ${this.messages.map((m) => { const isSelf = this.myClientID !== null && m.sender === this.myClientID; const cls = isSelf ? "lcp-msg lcp-right" : "lcp-msg lcp-left"; return html`<div class="${cls}"> <span class="lcp-sender">${m.sender}:</span> ${m.text} </div>`; })} </div> <div class="lcp-input-row"> <input class="lcp-input" type="text" maxlength="300" + aria-label="Chat message input" .value=${this.inputText} @input=${(e: Event) => (this.inputText = (e.target as HTMLInputElement).value)} @keydown=${(e: KeyboardEvent) => { if (e.key === "Enter") { e.preventDefault(); e.stopPropagation(); this.sendMessage(); } }} placeholder=${translateText("lobby_chat.placeholder")} /> - <button class="lcp-send" @click=${() => this.sendMessage()}> + <button class="lcp-send" @click=${() => this.sendMessage()} aria-label="Send message"> ${translateText("lobby_chat.send")} </button> </div> </div> `; }This improves usability for assistive technologies.
129-142: Optional: Guard against duplicate style injection.The style is injected at module scope, which is fine for typical builds. However, adding a guard prevents accidental duplication if the module is ever imported dynamically multiple times.
Apply this diff to add a simple guard:
-// Basic Tailwind-like classes, prefixed to avoid global conflicts -const style = document.createElement("style"); -style.textContent = ` -.lcp-container { display:flex; flex-direction:column; gap:8px; max-height:240px; } -.lcp-messages { overflow-y:auto; border:1px solid #444; border-radius:8px; padding:8px; height:180px; background:#111; color:#ddd; display:flex; flex-direction:column; gap:6px; } -.lcp-msg { font-size: 0.9rem; max-width: 80%; padding:6px 10px; border-radius:10px; background:#1b1b1b; } -.lcp-msg.lcp-left { align-self: flex-start; } -.lcp-msg.lcp-right { align-self: flex-end; background:#243b55; } -.lcp-sender { color:#9ae6b4; margin-right:4px; } -.lcp-input-row { display:flex; gap:8px; } -.lcp-input { flex:1; border-radius:8px; padding:6px 10px; color:#000; } -.lcp-send { border-radius:8px; padding:6px 12px; } -`; -document.head.appendChild(style); +// Basic Tailwind-like classes, prefixed to avoid global conflicts +if (!document.getElementById("lobby-chat-panel-styles")) { + const style = document.createElement("style"); + style.id = "lobby-chat-panel-styles"; + style.textContent = ` +.lcp-container { display:flex; flex-direction:column; gap:8px; max-height:240px; } +.lcp-messages { overflow-y:auto; border:1px solid #444; border-radius:8px; padding:8px; height:180px; background:#111; color:#ddd; display:flex; flex-direction:column; gap:6px; } +.lcp-msg { font-size: 0.9rem; max-width: 80%; padding:6px 10px; border-radius:10px; background:#1b1b1b; } +.lcp-msg.lcp-left { align-self: flex-start; } +.lcp-msg.lcp-right { align-self: flex-end; background:#243b55; } +.lcp-sender { color:#9ae6b4; margin-right:4px; } +.lcp-input-row { display:flex; gap:8px; } +.lcp-input { flex:1; border-radius:8px; padding:6px 10px; color:#000; } +.lcp-send { border-radius:8px; padding:6px 12px; } +`; + document.head.appendChild(style); +}This ensures styles are only injected once even in edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/components/LobbyChatPanel.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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/components/LobbyChatPanel.ts
🔇 Additional comments (7)
src/client/components/LobbyChatPanel.ts (7)
1-10: LGTM! Clean imports and interface.The imports are minimal and focused. The
ChatMessageinterface is appropriately simple for internal state management.
12-18: LGTM! Component setup follows Lit conventions.The state management and private property declarations are clean and appropriately typed.
35-39: LGTM! Proper cleanup in disconnectedCallback.The event listener cleanup is symmetric with
connectedCallbackand prevents memory leaks.
41-43: LGTM! Public API for EventBus injection.This method provides a clean way to explicitly set the EventBus, useful for testing and initialization scenarios.
57-63: LGTM! Lazy attachment logic is sound.The handler correctly re-attempts to capture the global EventBus and client ID when the bus becomes ready, with appropriate fallback for
myClientID.
65-84: LGTM! Message sending logic is robust.The method correctly handles:
- Empty input validation
- Lazy EventBus attachment
- Failure detection with user-visible warning
- 300-character limit enforcement matching server validation
- Input clearing only on success
124-126: LGTM! Light DOM usage is documented.The decision to use light DOM for style integration is clearly documented. The
lcp-class prefix mitigates naming conflicts.
- Change remaining old-style OutlineFilter constructor call - Fixes webpack build errors from missing pixi-filters package - Resolves CI failure in PR openfrontio#2514
Phantasm0009
force-pushed
the
lobby-chat-panel
branch
from
f1b6710 to
b16587d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
275-276: Consider a helper function to reduce duplication and removeas anycasts.The
as anycasts on allOutlineFilterusages bypass type checking. If the filter is working at runtime, this is acceptable, but the casts hide potential type mismatches. A small helper function can centralize the cast and make the code cleaner:// Add near top of file or in a shared utility function createOutlineFilter( thickness: number, color: number, ): PIXI.Filter { return new OutlineFilter(thickness, color) as unknown as PIXI.Filter; }Then replace usages:
-this.ghostUnit.container.filters = [ - new OutlineFilter(2, 0xff0000) as any, -]; +this.ghostUnit.container.filters = [createOutlineFilter(2, 0xff0000)];This keeps the workaround in one place and avoids scattering
as anythroughout the file.Also applies to: 293-297, 299-302, 499-500
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/StructureIconsLayer.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
- Replace @pixi/filter-outline with pixi-filters@6.1.4 - Update OutlineFilter import in StructureIconsLayer.ts - Move pixi-filters to devDependencies alongside pixi.js - Ensures compatibility with PixiJS v8.11.0 - Verified: builds compile successfully without errors
|
please review |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/Schemas.ts (1)
172-173: Remove redundant comment.The field name
chatEnabledis self-documenting. The comment adds no value.Apply this diff:
- // New: Enable in-lobby chat for private games chatEnabled: z.boolean().default(false),
🧹 Nitpick comments (2)
src/types/assets.d.ts (1)
1-55: Standardize variable names across ambient module declarations.Most declarations use
valueas the variable name, but*.png(line 2) and*.html(line 36) usecontent. While this doesn't affect functionality in ambient declarations, consistent naming improves code clarity and maintainability.Apply this diff to standardize on
value:declare module "*.png" { - const content: string; - export default content; + const value: string; + export default value; }declare module "*.html" { - const content: string; - export default content; + const value: string; + export default value; }src/global.d.ts (1)
3-7: Consider documenting the purpose and lifetime of global properties.Exposing
__eventBusand__usernameon the globalWindowinterface creates implicit dependencies that can complicate testing and reasoning about code. While this pattern enables the lobby chat event-driven architecture, adding JSDoc comments would clarify:
- When these properties are initialized
- Their expected lifetime (per-session, per-game, etc.)
- Whether they should be accessed directly or through helper functions
Example documentation:
declare global { interface Window { /** * Global event bus instance, initialized by ClientGameRunner. * Used for cross-component communication (e.g., lobby chat events). * Available after 'event-bus:ready' event. */ __eventBus?: EventBus; /** * Current user's display name, set during authentication/login. */ __username?: string; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
jest.config.ts(1 hunks)resources/lang/en.json(1 hunks)src/client/ClientGameRunner.ts(4 hunks)src/client/components/LobbyChatPanel.ts(1 hunks)src/core/Schemas.ts(6 hunks)src/global.d.ts(1 hunks)src/server/MapPlaylist.ts(1 hunks)src/types/assets.d.ts(1 hunks)tests/global.d.ts(1 hunks)tsconfig.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/client/ClientGameRunner.ts
- jest.config.ts
- resources/lang/en.json
- src/client/components/LobbyChatPanel.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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/server/MapPlaylist.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
📚 Learning: 2025-06-07T13:15:55.439Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/types/assets.d.tssrc/global.d.tstests/global.d.ts
🧬 Code graph analysis (1)
src/global.d.ts (1)
src/core/EventBus.ts (1)
EventBus(7-44)
🔇 Additional comments (9)
tests/global.d.ts (1)
1-3: LGTM!The import of asset type declarations and module boundary establishment is correct. This ensures tests have access to the centralized asset type definitions.
src/core/Schemas.ts (4)
93-94: LGTM! Type unions updated correctly.The new lobby chat message types are properly added to the ClientMessage and ServerMessage unions.
Also applies to: 101-102
119-120: LGTM! Type exports follow the existing pattern.The new message types are correctly inferred from their schemas.
531-535: LGTM! Client schema looks correct.The ClientLobbyChatSchema follows existing patterns with proper validation for clientID and text.
512-512: LGTM! Discriminated unions updated correctly.The new schemas are properly added to both ServerMessageSchema and ClientMessageSchema discriminated unions with the correct discriminator field.
Also applies to: 580-580
src/server/MapPlaylist.ts (1)
97-97: LGTM! Correct default for public games.Setting
chatEnabled: falsefor public playlist games aligns with the PR objective of providing chat only for private lobbies.tsconfig.json (3)
6-12: LGTM! Appropriate lib entries for browser-based chat.The lib array additions are well-suited for the lobby chat feature. The DOM libraries support the LitElement UI components, and ES2021.Intl aligns with the internationalization work mentioned in the PR.
40-41: LGTM! Standard test infrastructure setup.Including
__mocks__/**/*ensures the test mocks (like the jose mock) are properly recognized by TypeScript and Jest.
28-28: Verify whether skipLibCheck can be safely removed or is masking real type errors.The project uses modern TypeScript (5.7.2) and recent dependency versions (webpack 5, jest 30, eslint 9), which typically have good type definitions. However, no
@ts-ignoreor@ts-nocheckcomments appear in the codebase, making it unclear whetherskipLibCheck: trueis suppressing legitimate type errors from dependencies or if it's simply an unnecessary legacy setting.To confirm: run
tsc --noEmitin your local environment withskipLibCheckset tofalse. If the build succeeds, remove the flag. If type errors appear, evaluate whether they come from outdated dependencies or genuine conflicts that need resolution.
|
Hello! I labled this up and assigned it to a milestone so it doesnt fall through the cracks Will send to evan for another review shortly |
iiamlewis
added
Release Blocker
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
|
@Phantasm0009 Can you fix the merge conflicts? This is on the list to be reviewed still |
I'm currently on vacation so i don't have access to my laptop right now. I'll finish it immediately when i come back. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
jest.config.tsresources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/client/SinglePlayerModal.tssrc/client/Transport.tssrc/core/Schemas.tssrc/server/GameManager.tssrc/server/GameServer.tssrc/server/MapPlaylist.tstests/util/Setup.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/server/GameManager.ts
- src/client/SinglePlayerModal.ts
- src/server/GameServer.ts
- src/client/Transport.ts
- src/core/Schemas.ts
- jest.config.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/ClientGameRunner.tssrc/client/HostLobbyModal.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/ClientGameRunner.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/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/server/MapPlaylist.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/ClientGameRunner.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/ClientGameRunner.tstests/util/Setup.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/ClientGameRunner.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
tests/util/Setup.ts
🧬 Code graph analysis (2)
src/client/ClientGameRunner.ts (1)
src/core/Schemas.ts (2)
ServerMessage(99-106)ServerLobbyChatMessage(124-124)
src/client/HostLobbyModal.ts (2)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)
🪛 Biome (2.1.2)
src/server/MapPlaylist.ts
[error] 98-98: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
🪛 GitHub Actions: 🧪 CI
src/client/HostLobbyModal.ts
[warning] 1-1: Code style issues found in HostLobbyModal.ts. Run 'npx prettier --write .' to fix.
🔇 Additional comments (9)
tests/util/Setup.ts (1)
64-65: LGTM! Appropriate test defaults.The new config fields
chatEnabledanddisableNPCsuse sensible defaults for the test environment. SettingchatEnabled: falseensures tests don't rely on chat infrastructure, anddisableNPCs: falsemaintains default NPC behavior.resources/lang/en.json (1)
549-554: LGTM! Clear and concise localization strings.The lobby chat UI strings are well-written and appropriate. The keys cover all necessary UI elements: panel title, input placeholder, toggle label, and send button.
src/client/HostLobbyModal.ts (3)
28-28: LGTM!Import follows the existing pattern for component imports.
58-58: LGTM!State declaration follows the component pattern with an appropriate default value.
594-606: LGTM! Conditional rendering and config payload are correct.The lobby chat panel is properly conditionally rendered when
chatEnabledis true, and thechatEnabledfield is correctly included in the game configuration payload sent to the server.Note: A pipeline failure indicates a Prettier formatting issue. Run
npx prettier --write .to fix.Also applies to: 814-814
src/client/ClientGameRunner.ts (4)
10-10: LGTM!Import of
ServerLobbyChatMessageis necessary for the type guard implementation.
68-78: LGTM! Solid type guard implementation.The type guard properly validates the message structure at runtime before dispatching the event. This addresses the past review concern about validating message fields.
166-182: LGTM! Proper validation and error handling.The lobby chat message handling correctly uses the type guard to validate messages before dispatching events. Error logging for malformed messages is a good defensive practice. This fully addresses the past review concern about validating message fields.
90-94: TypeScript declarations for window globals are properly defined.The code correctly exposes
__eventBusand__usernameon thewindowobject with proper type declarations insrc/global.d.ts. The declaration merging pattern is correctly implemented:declare global { interface Window { __eventBus?: EventBus; __username?: string; } }Both properties are typed and optional, matching the runtime behavior. The past concern about
__clientIDremains addressed (not exposed).
github-project-automation
bot
moved this from Final Review
to Development
in OpenFront Release Management
|
@Phantasm0009 this has been set to draft due to conflicts, please reopen it when ready |
…tegrated both features: lobby chat and spawn immunity duration
Phantasm0009
force-pushed
the
lobby-chat-panel
branch
from
2942f67 to
b78fecc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tsconfig.json (1)
22-27: Remove the duplicateskipLibCheckkey.The property
skipLibCheckappears twice (lines 22 and 27). When a key is defined multiple times in JSON, only the last definition takes effect, making line 22 redundant.🔎 Proposed fix
"esModuleInterop": true, "experimentalDecorators": true, - "skipLibCheck": true, "resolveJsonModule": true, "strictNullChecks": true, "useDefineForClassFields": false, "strictPropertyInitialization": false, "skipLibCheck": true,
♻️ Duplicate comments (2)
src/core/Schemas.ts (1)
519-524: UseUsernameSchemafor the username field.The
usernamefield uses plainz.string()without validation. The codebase already definesUsernameSchema(lines 225-229) with specific validation rules. Using it here ensures consistent username validation across all schemas.Proposed fix
export const ServerLobbyChatSchema = z.object({ type: z.literal("lobby_chat"), - username: z.string(), + username: UsernameSchema, isHost: z.boolean(), text: SafeString.max(300), });src/client/ClientGameRunner.ts (1)
90-94: Missing TypeScript declarations for window properties causes build failure.The pipeline shows
TS2339: Property '__eventBus' does not exist on type 'Window & typeof globalThis'. Add type declarations to fix this.Proposed fix
Add this declaration before the
joinLobbyfunction or in a separate.d.tsfile:+declare global { + interface Window { + __eventBus: EventBus; + __username: string; + } +} + export function joinLobby(
🧹 Nitpick comments (2)
src/types/assets.d.ts (1)
1-55: Standardize the variable name across all declarations.The declarations use inconsistent variable names:
contentfor*.pngand*.html, butvaluefor all other extensions. While this doesn't affect functionality, it reduces readability. Consider standardizing onvaluethroughout for consistency.🔎 Proposed fix to standardize on `value`
declare module "*.png" { - const content: string; - export default content; + const value: string; + export default value; } declare module "*.jpg" { const value: string; export default value; } declare module "*.webp" { const value: string; export default value; } declare module "*.jpeg" { const value: string; export default value; } declare module "*.svg" { const value: string; export default value; } declare module "*.bin" { const value: string; export default value; } declare module "*.md" { const value: string; export default value; } declare module "*.txt" { const value: string; export default value; } declare module "*.html" { - const content: string; - export default content; + const value: string; + export default value; } declare module "*.xml" { const value: string; export default value; } declare module "*.mp3" { const value: string; export default value; } declare module "*.wav" { const value: string; export default value; } declare module "*.ogg" { const value: string; export default value; }src/client/components/LobbyChatPanel.ts (1)
126-181: Consider moving style injection into component lifecycle.Styles are injected at module load time, meaning they're added to
document.headeven if the component is never instantiated. This is minor but could be cleaner.A past reviewer also suggested using Tailwind instead of raw CSS for consistency with the rest of the codebase.
Option: Inject styles on first connectedCallback
+let stylesInjected = false; + +function injectStyles() { + if (stylesInjected) return; + stylesInjected = true; + const style = document.createElement("style"); + style.textContent = `/* ... */`; + document.head.appendChild(style); +} + @customElement("lobby-chat-panel") export class LobbyChatPanel extends LitElement { // ... connectedCallback(): void { super.connectedCallback(); + injectStyles(); document.addEventListener("lobby-chat:message", this.onIncoming as any); // ... } } - -const style = document.createElement("style"); -// ... remove module-level injection
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.husky/pre-commit__mocks__/jose.jseslint.config.jsresources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.tssrc/client/SinglePlayerModal.tssrc/client/Transport.tssrc/client/components/LobbyChatPanel.tssrc/core/Schemas.tssrc/server/GameManager.tssrc/server/GameServer.tssrc/server/MapPlaylist.tssrc/types/assets.d.tstests/LobbyChatPanel.test.tstests/LobbyChatSchemas.test.tstests/util/Setup.tstsconfig.json
🚧 Files skipped from review as they are similar to previous changes (9)
- src/client/Transport.ts
- src/client/SinglePlayerModal.ts
- src/server/MapPlaylist.ts
- src/server/GameManager.ts
- tests/LobbyChatPanel.test.ts
- src/client/JoinPrivateLobbyModal.ts
- tests/util/Setup.ts
- resources/lang/en.json
- eslint.config.js
🧰 Additional context used
🧠 Learnings (14)
📓 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.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
📚 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/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/client/components/LobbyChatPanel.tssrc/server/GameServer.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/ClientGameRunner.tssrc/client/components/LobbyChatPanel.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/client/ClientGameRunner.tssrc/server/GameServer.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/ClientGameRunner.tssrc/client/HostLobbyModal.tssrc/server/GameServer.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/ClientGameRunner.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/ClientGameRunner.tssrc/client/components/LobbyChatPanel.tssrc/server/GameServer.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the ?s=xxxxx URL suffix used in lobby URLs is purely for cache-busting platform previews (e.g., Discord, WhatsApp, x.com) and is not used by the join logic. The join flow ignores the suffix value, so regenerating it via updateUrlWithSuffix() on configuration changes will not break existing shared URLs; it only prompts platforms to refresh preview metadata. Treat the suffix as non-functional for join behavior and ensure any related changes preserve that invariant.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-06-14T00:56:15.437Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Cosmetics.ts:1-1
Timestamp: 2025-06-14T00:56:15.437Z
Learning: The `import { base64url } from "jose";` syntax works correctly in ESM environments for the jose library, contrary to potential assumptions about needing specific ESM import paths like "jose/util/base64url".
Applied to files:
__mocks__/jose.js
📚 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/server/GameServer.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
📚 Learning: 2025-06-07T13:15:55.439Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 786
File: src/core/Util.ts:4-4
Timestamp: 2025-06-07T13:15:55.439Z
Learning: In the OpenFrontIO codebase, JSON files should be imported using standard import syntax without import attributes, as the TypeScript configuration supports resolveJsonModule and the codebase already uses this pattern successfully in files like src/client/Cosmetic.ts.
Applied to files:
src/types/assets.d.ts
🧬 Code graph analysis (4)
src/client/ClientGameRunner.ts (1)
src/core/Schemas.ts (2)
ServerMessage(101-108)ServerLobbyChatMessage(126-126)
src/client/HostLobbyModal.ts (2)
src/client/LangSelector.ts (1)
translateText(222-242)src/client/Utils.ts (1)
translateText(92-147)
src/client/components/LobbyChatPanel.ts (2)
src/core/EventBus.ts (1)
EventBus(7-44)src/client/Transport.ts (1)
SendLobbyChatEvent(181-183)
tests/LobbyChatSchemas.test.ts (1)
src/core/Schemas.ts (3)
GameConfigSchema(169-190)ClientLobbyChatSchema(552-556)ServerLobbyChatSchema(519-524)
🪛 Biome (2.1.2)
tsconfig.json
[error] 22-22: The key skipLibCheck was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🪛 GitHub Actions: 🧪 CI
src/client/ClientGameRunner.ts
[error] 90-90: TS2339: Property '__eventBus' does not exist on type 'Window & typeof globalThis'.
src/core/Schemas.ts
[warning] 1-1: Module 'resources/countries.json' imported with no attributes, but previously imported with 'type': 'json'. Ensure import attributes are consistent.
tests/LobbyChatSchemas.test.ts
[error] 9-9: ZodError: [{"expected": "boolean", "code": "invalid_type", "path": ["disableNations"], "message": "Invalid input: expected boolean, received undefined"}]
tsconfig.json
[warning] 27-27: Duplicate key 'skipLibCheck' in object literal. The original key is defined at tsconfig.json:22:4.
[warning] 22-22: Duplicate key 'skipLibCheck' in object literal.
🪛 GitHub Check: 🔬 Test
tests/LobbyChatSchemas.test.ts
[failure] 9-9: tests/LobbyChatSchemas.test.ts > Lobby Chat Schemas > GameConfigSchema applies default chatEnabled false
ZodError: [
{
"expected": "boolean",
"code": "invalid_type",
"path": [
"disableNations"
],
"message": "Invalid input: expected boolean, received undefined"
}
]
❯ tests/LobbyChatSchemas.test.ts:9:34
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { _zod: { traits: { constructor: 'Function', has: 'Function', add: 'Function', delete: 'Function', clear: 'Function', entries: 'Function', forEach: 'Function', size: 2, values: 'Function', keys: 'Function' }, constr: 'Function', def: [ { expected: 'boolean', code: 'invalid_type', path: [ 'disableNations' ], message: 'Invalid input: expected boolean, received undefined' } ], deferred: [] }, issues: [ { expected: 'boolean', code: 'invalid_type', path: [ 'disableNations' ], message: 'Invalid input: expected boolean, received undefined' } ], format: 'Function', flatten: 'Function', addIssue: 'Function', addIssues: 'Function', isEmpty: false }
🔇 Additional comments (17)
src/client/HostLobbyModal.ts (3)
416-436: LGTM! Lobby chat toggle follows existing patterns.The checkbox implementation is consistent with other game options (instant build, random spawn, etc.) and properly triggers config updates.
642-653: LGTM! Conditional rendering is clean.The lobby chat panel is rendered only when enabled, following idiomatic Lit patterns used elsewhere in the modal.
890-890: LGTM! Config includes chat enablement.The
chatEnabledflag is properly included in the game configuration payload sent to the server.src/server/GameServer.ts (2)
124-129: LGTM! Public lobby chat restriction enforced server-side.The enforcement correctly disables chat for public lobbies regardless of the client-provided value, ensuring the business rule is applied at the authoritative layer.
450-466: LGTM! Lobby chat message handling is correct.The implementation:
- Validates game phase (Lobby only)
- Checks chatEnabled flag
- Determines host status using lobbyCreatorID
- Uses the already-parsed and validated
clientMsg.text- Broadcasts to all active clients with username and host designation
tests/LobbyChatSchemas.test.ts (2)
27-44: LGTM! Client schema tests cover valid and invalid cases.The tests properly validate that ClientLobbyChatSchema accepts valid messages and rejects text exceeding 300 characters.
46-63: LGTM! Server schema tests cover valid and invalid cases.The tests properly validate that ServerLobbyChatSchema requires username and isHost fields and rejects incomplete messages.
src/client/ClientGameRunner.ts (2)
68-78: Type guard implementation looks good.The guard properly validates the
lobby_chatmessage structure before use. The cast toRecord<string, unknown>followed by property checks is a clean approach that addresses prior feedback about runtime validation.
166-182: Lobby chat message handling is solid.Good use of the type guard before dispatching, with proper error logging for malformed messages. The CustomEvent with
bubbles: true, composed: trueallows the LobbyChatPanel component to receive these events correctly.src/client/components/LobbyChatPanel.ts (5)
1-11: Clean component setup with proper typing.The
ChatMessageinterface is simple and clear. Good use of LitElement decorators for reactive state management.
21-36: Event listener cleanup is properly handled.Good pattern: listeners added in
connectedCallbackare removed indisconnectedCallback. This prevents memory leaks when the component is removed from DOM.
42-52: Auto-scroll timing issue resolved.Using
await this.updateCompletebefore querying the container ensures the DOM has rendered the new message. This addresses prior feedback correctly.
62-78: Silent failure now properly handled.The warning log when EventBus is unavailable helps with debugging. Input text is preserved when send fails, so users don't lose their message.
80-119: Message rendering and alignment work as expected.Local messages align right with a tinted background; remote messages align left. The host indicator
(Host)is appended correctly. The Enter key handling withstopPropagationprevents form submission issues.src/core/Schemas.ts (3)
178-179: Good addition of chatEnabled field with sensible default.Setting
default(false)ensures existing game configs remain compatible. The comment is helpful context.
552-556: ClientLobbyChatSchema is well-structured.Uses
IDfor clientID andSafeString.max(300)for text, matching server-side limits. Clean and consistent with other client message schemas.
99-108: Message union updates look correct.Both
ClientMessageandServerMessageunions now include the lobby chat message types, and the corresponding discriminated union schemas are updated at lines 533 and 601.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/vite-env.d.ts (1)
28-33: Single underscore prefix is more idiomatic than double underscore.The double-underscore (
__) prefix is typically reserved for language-level properties (like__proto__). For internal globals, a single underscore (_eventBus,_username) is more standard in TypeScript/JavaScript.Good news: all code accessing these properties already handles
undefinedcorrectly usingifchecks and nullish coalescing operators (??).Suggested naming improvement
declare global { interface Window { - __eventBus?: import("../core/EventBus").EventBus; - __username?: string; + _eventBus?: import("../core/EventBus").EventBus; + _username?: string; } }This change requires updating assignments in
ClientGameRunner.tsand usages inLobbyChatPanel.tsto match the new names.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/vite-env.d.tstests/GameInfoRanking.test.tstests/LobbyChatSchemas.test.tstsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/LobbyChatSchemas.test.ts
🧰 Additional context used
🧠 Learnings (2)
📓 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-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
tests/GameInfoRanking.test.ts
🔇 Additional comments (1)
tests/GameInfoRanking.test.ts (1)
30-30: LGTM! Test data updated to match schema.The addition of
chatEnabled: falsecorrectly aligns the mock configuration with the new GameConfigSchema field introduced in this PR.
|
Im done I fixed everything and the merge conflicts too |
9 participants
Resolves #2490
Description:
Adds a new Lobby Chat feature for private game lobbies. When enabled by the host, players can communicate in a live chat panel before the match begins. Useful for confirming teams, rules, and ready-checks without needing external tools like Discord.
Key Features:
translateText()with keys added toen.json.event-bus:ready, with lazy fallback.Please complete the following:
translateText()and I've added it to theen.jsonfilePlease put your Discord username so you can be contacted if a bug or regression is found:
.fruitymctooty