Enabled the @typescript-eslint/no-unused-expressions eslint rule
#2014
Conversation
|
|
WalkthroughRemoved the explicit disable of Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant Main as Main.ts
participant Registry as Modal Registry
participant Modal as Specific Modal
rect rgba(100,150,220,0.08)
U->>Main: request open modal
Main->>Registry: lookup modal element
alt exists && instanceof expected
Main->>Modal: show()/open()
else missing or wrong type
Main->>Main: console.warn("modal missing or wrong type")
end
end
sequenceDiagram
participant Row as EventsDisplay Row
participant Bus as EventBus
Row->>Row: user clicks row
alt event.focusID present
Row->>Bus: emit GoToPlayerEvent(focusID)
end
alt event.unitView present
Row->>Bus: emit GoToUnitEvent(unitView)
end
sequenceDiagram
participant Renderer as RailroadExecution
participant From as fromBuilding
participant To as toBuilding
Renderer->>Renderer: prepare redrawBuildings()
alt fromUnit active
Renderer->>From: touch()
end
alt toUnit active
Renderer->>To: touch()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 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: 9
🧹 Nitpick comments (3)
src/client/graphics/layers/EventsDisplay.ts (1)
1038-1051: Drop redundant inner checks in onClick closuresYou already branch on
event.focusID/event.unitViewabove. The innerifadds noise without safety.- onClick: () => { - if (event.focusID) - this.emitGoToPlayerEvent(event.focusID); - }, + onClick: () => + this.emitGoToPlayerEvent(event.focusID!), ... - onClick: () => { - if (event.unitView) - this.emitGoToUnitEvent( - event.unitView, - ); - }, + onClick: () => + this.emitGoToUnitEvent(event.unitView!),If you prefer to avoid non-null assertions, capture into a const before the template map.
src/client/Main.ts (2)
108-111: Avoid unsafe cast; rely on runtime narrowingCasting to
NewsModaland then checkinginstanceofis redundant and can hide null at compile time.- const newsModal = document.querySelector("news-modal") as NewsModal; - if (!newsModal || !(newsModal instanceof NewsModal)) { + const newsModal = document.querySelector("news-modal"); + if (!(newsModal instanceof NewsModal)) { console.warn("News modal element not found"); }
96-430: Reduce repetition: introduce a tiny helper to fetch-and-narrow custom elementsYou repeat “query + instanceof + warn” many times. A small helper improves readability and safety.
function getEl<T extends Element>( selector: string, Ctor: abstract new (...args: any[]) => T, name: string, ): T | null { const el = document.querySelector(selector); if (el instanceof Ctor) return el; console.warn(`${name} element not found`); return null; } // Usage const spModal = getEl("single-player-modal", SinglePlayerModal, "Singleplayer modal"); const hlpModal = getEl("help-modal", HelpModal, "Help modal");This keeps code clean and idiomatic TS without extra libs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
eslint.config.js(1 hunks)src/client/Main.ts(10 hunks)src/client/graphics/layers/EventsDisplay.ts(2 hunks)src/core/execution/RailroadExecution.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Main.ts
🔇 Additional comments (2)
src/core/execution/RailroadExecution.ts (1)
167-169: Guardunitaccess to avoid runtime errors
Replace direct calls with local variables and optional chaining:- if (this.railRoad.from.unit.isActive()) this.railRoad.from.unit.touch(); - if (this.railRoad.to.unit.isActive()) this.railRoad.to.unit.touch(); + const fromUnit = this.railRoad.from.unit; + if (fromUnit?.isActive()) fromUnit.touch(); + const toUnit = this.railRoad.to.unit; + if (toUnit?.isActive()) toUnit.touch();If
unitis always non-null here by contract, you can omit the?.checks and treat this as a nit. Otherwise, keep the guards.src/client/Main.ts (1)
575-577: Nice: explicit guard before showing starting modalClear and robust. Matches the lint rule’s intent.
Michal-Martinek
force-pushed
the
eslint
branch
from
9340f8f to
e701dc9
Compare
Michal-Martinek
force-pushed
the
eslint
branch
from
e701dc9 to
36bd029
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
src/client/Main.ts (8)
166-171: Still unsafe: spModal.open() can run when element is missingYou warn but the click handler calls
open()unguarded. This can crash at runtime.Minimal, typesafe fix:
- const spModal = document.querySelector( - "single-player-modal", - ) as SinglePlayerModal; + const spModal = document.querySelector( + "single-player-modal", + ) as SinglePlayerModal | null; @@ - if (this.usernameInput?.isValid()) { - spModal.open(); - } + if (this.usernameInput?.isValid()) { + spModal?.open(); + }Also applies to: 175-179
187-190: Same pattern: hlpModal.open() unguardedThe click handler can call
open()on a missing element.- const hlpModal = document.querySelector("help-modal") as HelpModal; + const hlpModal = document.querySelector("help-modal") as HelpModal | null; @@ - helpButton.addEventListener("click", () => { - hlpModal.open(); - }); + helpButton.addEventListener("click", () => hlpModal?.open());Also applies to: 193-195
197-203: Same pattern: flagInputModal.open() unguardedCan throw when the element is absent.
- const flagInputModal = document.querySelector( - "flag-input-modal", - ) as FlagInputModal; + const flagInputModal = document.querySelector( + "flag-input-modal", + ) as FlagInputModal | null; @@ - flgInput.addEventListener("click", () => { - flagInputModal.open(); - }); + flgInput.addEventListener("click", () => flagInputModal?.open());Also applies to: 206-208
210-218: Immediate usages of patternsModal can crash; return early after warnYou set properties and call methods right after a warn, which will throw if the element is missing or wrong type. Early-return keeps later code safe.
- this.patternsModal = document.querySelector( + this.patternsModal = document.querySelector( "territory-patterns-modal", ) as TerritoryPatternsModal; if ( !this.patternsModal || !(this.patternsModal instanceof TerritoryPatternsModal) ) { console.warn("Territory patterns modal element not found"); + return; }Also applies to: 224-228
230-238: tokenLoginModal is used later without checks
this.tokenLoginModal.open(token)in hash handlers will throw if the element is missing.Two small, local fixes:
- this.tokenLoginModal = document.querySelector( - "token-login", - ) as TokenLoginModal; + this.tokenLoginModal = document.querySelector( + "token-login", + ) as TokenLoginModal | null;- this.tokenLoginModal.open(token); + if (this.tokenLoginModal instanceof TokenLoginModal) { + this.tokenLoginModal.open(token); + } else { + console.warn("Token login modal element not found"); + }- this.tokenLoginModal.open(token); + if (this.tokenLoginModal instanceof TokenLoginModal) { + this.tokenLoginModal.open(token); + } else { + console.warn("Token login modal element not found"); + }Also applies to: 478-479, 496-498
346-351: settingsModal.open() should be guarded at bind timeAttach the listener only when the element is valid.
- const settingsModal = document.querySelector( - "user-setting", - ) as UserSettingModal; - if (!settingsModal || !(settingsModal instanceof UserSettingModal)) { - console.warn("User settings modal element not found"); - } - document - .getElementById("settings-button") - ?.addEventListener("click", () => { - settingsModal.open(); - }); + const settingsEl = document.querySelector("user-setting"); + if (settingsEl instanceof UserSettingModal) { + document.getElementById("settings-button")?.addEventListener("click", () => { + settingsEl.open(); + }); + } else { + console.warn("User settings modal element not found"); + }Also applies to: 352-356
358-363: hostModal.open() should be guardedSame issue:
open()called even if element is missing.- const hostModal = document.querySelector( - "host-lobby-modal", - ) as HostPrivateLobbyModal; - if (!hostModal || !(hostModal instanceof HostPrivateLobbyModal)) { - console.warn("Host private lobby modal element not found"); - } + const hostModal = document.querySelector("host-lobby-modal"); + if (!(hostModal instanceof HostPrivateLobbyModal)) { + console.warn("Host private lobby modal element not found"); + } @@ - hostModal.open(); + if (hostModal instanceof HostPrivateLobbyModal) hostModal.open();Also applies to: 366-371
373-378: joinModal is assumed present later; enforce invariant or guard usesDownstream code calls
open()/close()without checks. Fail fast to keep the property non-null, or make it nullable and guard every use (more churn). Recommend fail-fast.- this.joinModal = document.querySelector( + this.joinModal = document.querySelector( "join-private-lobby-modal", ) as JoinPrivateLobbyModal; - if (!this.joinModal || !(this.joinModal instanceof JoinPrivateLobbyModal)) { - console.warn("Join private lobby modal element not found"); - } + if (!(this.joinModal instanceof JoinPrivateLobbyModal)) { + throw new Error("JoinPrivateLobbyModal element not found"); + }Also applies to: 385-388
🧹 Nitpick comments (2)
src/client/Main.ts (2)
107-111: Simplify the guard and avoid unsafeascast
newsModal ||is redundant;null instanceof NewsModalis already false. Also prefer runtime-safe narrowing withoutas.- const newsModal = document.querySelector("news-modal") as NewsModal; - if (!newsModal || !(newsModal instanceof NewsModal)) { + const newsEl = document.querySelector("news-modal"); + if (!(newsEl instanceof NewsModal)) { console.warn("News modal element not found"); }
575-577: LGTM: safe show for GameStartingModalGood guard; avoids calling
show()on a wrong/missing element. Minor nit:instanceofalready handles null, so the leading truthy check is redundant.- if (startingModal && startingModal instanceof GameStartingModal) { + if (startingModal instanceof GameStartingModal) { startingModal.show(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/Main.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Michal-Martinek
PR: openfrontio/OpenFrontIO#2014
File: eslint.config.js:43-43
Timestamp: 2025-09-06T07:10:42.249Z
Learning: When analyzing potential no-unused-expressions violations, always examine the full context. Expressions used in template literals, filter predicates, assignments, or other consuming contexts are not violations even if they use short-circuit operators like &&.
Learnt from: Michal-Martinek
PR: openfrontio/OpenFrontIO#2014
File: eslint.config.js:43-43
Timestamp: 2025-09-06T07:10:42.248Z
Learning: The `typescript-eslint/no-unused-expressions` ESLint rule only flags standalone expression statements that don't consume their results or have side effects. Expressions that are part of return statements, assignments, or other consuming contexts are not violations of this rule.
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/Main.ts
Michal-Martinek
force-pushed
the
eslint
branch
from
735bc50 to
bd21588
Compare
|
@evanpelle @scottanderson review please, i have this opened for some weeks now. |
|
looks like there's a merge conflict |
Michal-Martinek
force-pushed
the
eslint
branch
from
bd21588 to
66f2eb4
Compare
|
@evanpelle sorry for that, just rebased to resolve the conflict. |
TheGiraffe3
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
|
As long as the current version of the PR passes, there won't be problems. |
TheGiraffe3
added
the
DevOps - CICD
Currently raising `console.warn` on fauilure
Michal-Martinek
force-pushed
the
eslint
branch
from
66f2eb4 to
086a58f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/graphics/layers/StructureIconsLayer.ts (1)
258-260: Conversion looks good.The short-circuit expression has been correctly converted to an explicit if statement, making the code more readable and meeting the PR objectives.
Optional simplification:
Since
containeris typed asPIXI.Container(non-nullable) in theghostUnittype definition (lines 58-62), the check could be simplified:- if (this.ghostUnit?.container) { + if (this.ghostUnit) { this.ghostUnit.container.filters = []; }The current check works fine but is slightly redundant—
containeris always present whenghostUnitis not null.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eslint.config.js(0 hunks)src/client/Main.ts(10 hunks)src/client/graphics/layers/EventsDisplay.ts(2 hunks)src/client/graphics/layers/StructureIconsLayer.ts(1 hunks)src/core/execution/RailroadExecution.ts(1 hunks)
💤 Files with no reviewable changes (1)
- eslint.config.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/graphics/layers/EventsDisplay.ts
- src/core/execution/RailroadExecution.ts
- src/client/Main.ts
4 participants
Description:
@typescript-eslint/no-unused-expressionseslint rule #1790&&changed to properifstatementsA instanceof B;expressions now emit warningsPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
michal7952