Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralized per-event FX gating in FxLayer and added construction tracking; many new sound assets and enum entries registered; events and UI (chat, alliances, radial menu, unit/structure events) now trigger corresponding sound effects while visual FX remain gated per-event. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as GameEvent
participant Fx as FxLayer
participant EventsDisp as EventsDisplay
participant UI as RadialMenu
participant Sound as SoundManager
participant Audio as AudioSystem
Event->>Fx: deliver event (shell/train/warship/structure/conquest)
Fx->>Fx: fxEnabled()? (per-event)
alt fxEnabled == true
Fx->>Fx: create visual FX (explosion, shockwave, building FX)
end
Fx->>Sound: play(launch/hit/build sounds)
EventsDisp->>Sound: play(Message / AllianceSuggested / AllianceBroken)
UI->>Sound: play(Click) on item click
Sound->>Audio: load/decode & play asset
Audio-->>Sound: playback started
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/FxLayer.ts`:
- Line 106: The visual FX guard in FxLayer.tick (userSettings()?.fxLayer())
currently prevents all SoundManager.playSoundEffect calls (e.g.,
SoundEffect.WarshipShot and the other calls at the locations you noted), which
mutes combat audio when visual FX are off; change this by decoupling sound
playback from the fxLayer gate: either move the SoundManager.playSoundEffect
calls outside the fxLayer conditional or wrap them with a separate sound setting
check (e.g., userSettings()?.sfxEnabled() or userSettings()?.sfxVolume() > 0)
and use that guard before calling SoundManager.playSoundEffect (update all
instances: the call at SoundEffect.WarshipShot plus the calls at the other noted
locations). Ensure you reference FxLayer.tick and SoundManager.playSoundEffect
when making the change so audio respects the SFX setting independently of visual
FX.
In `@src/client/sound/SoundManager.ts`:
- Around line 6-14: Prettier formatting errors are causing CI to fail for
SoundManager.ts; run the project's Prettier formatter (e.g., npx prettier
--write) on this file to fix import spacing/line breaks for the top imports
(atomHitSound, atomLaunchSound, hydrogenHitSound, hydrogenLaunchSound,
mirvLaunchSound, samHitSound, samShootSound, warshipShotSound, warshipLostSound)
and also reformat the block around lines 58-66 referenced in the review so the
file conforms to the repo's Prettier rules before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca5aea94-9f3e-4b4a-a425-cd85845e3d8a
⛔ Files ignored due to path filters (19)
resources/sounds/effects/alliance-broken.mp3is excluded by!**/*.mp3resources/sounds/effects/alliance-suggested.mp3is excluded by!**/*.mp3resources/sounds/effects/atom-hit.mp3is excluded by!**/*.mp3resources/sounds/effects/atom-launch.mp3is excluded by!**/*.mp3resources/sounds/effects/build-city.mp3is excluded by!**/*.mp3resources/sounds/effects/build-defense-post.mp3is excluded by!**/*.mp3resources/sounds/effects/build-port.mp3is excluded by!**/*.mp3resources/sounds/effects/build-warship.mp3is excluded by!**/*.mp3resources/sounds/effects/click.mp3is excluded by!**/*.mp3resources/sounds/effects/hydrogen-hit.mp3is excluded by!**/*.mp3resources/sounds/effects/hydrogen-launch.mp3is excluded by!**/*.mp3resources/sounds/effects/message.mp3is excluded by!**/*.mp3resources/sounds/effects/mirv-launch.mp3is excluded by!**/*.mp3resources/sounds/effects/sam-built.mp3is excluded by!**/*.mp3resources/sounds/effects/sam-hit.mp3is excluded by!**/*.mp3resources/sounds/effects/sam-shoot.mp3is excluded by!**/*.mp3resources/sounds/effects/silo-built.mp3is excluded by!**/*.mp3resources/sounds/effects/warship-lost.mp3is excluded by!**/*.mp3resources/sounds/effects/warship-shot.mp3is excluded by!**/*.mp3
📒 Files selected for processing (2)
src/client/graphics/layers/FxLayer.tssrc/client/sound/SoundManager.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
camclark
force-pushed
the
feature/fx-sound-effects
branch
from
e976d34 to
9b4bb9a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/graphics/layers/FxLayer.ts (2)
120-134:⚠️ Potential issue | 🟠 MajorMissing shell impact sound effect.
Per PR objectives, warship shell impacts should play
warship-shot.mp3. AddSoundManager.playSoundEffect(SoundEffect.WarshipShot)when the shell reaches its target.Proposed fix
onShellEvent(unit: UnitView) { if (!unit.isActive()) { if (unit.reachedTarget() && this.fxEnabled()) { const x = this.game.x(unit.lastTile()); const y = this.game.y(unit.lastTile()); const explosion = new SpriteFx( this.animatedSpriteLoader, x, y, FxType.MiniExplosion, ); this.allFx.push(explosion); } + if (unit.reachedTarget()) { + SoundManager.playSoundEffect(SoundEffect.WarshipShot); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/FxLayer.ts` around lines 120 - 134, The onShellEvent handler currently creates a MiniExplosion SpriteFx when a UnitView shell reaches its target but doesn't play the impact sound; update onShellEvent so that when unit.reachedTarget() && this.fxEnabled() succeeds you also call SoundManager.playSoundEffect(SoundEffect.WarshipShot) (alongside creating the SpriteFx and pushing to this.allFx) to trigger the warship shell impact audio; ensure you reference the existing SoundManager and SoundEffect enums when adding this call.
185-207:⚠️ Potential issue | 🟠 MajorMissing warship destruction sound effect.
Per PR objectives, warship destruction should play
warship-lost.mp3. AddSoundManager.playSoundEffect(SoundEffect.WarshipLost)outside thefxEnabled()guard.Proposed fix
onWarshipEvent(unit: UnitView) { - if (!unit.isActive() && this.fxEnabled()) { + if (!unit.isActive()) { + SoundManager.playSoundEffect(SoundEffect.WarshipLost); + if (!this.fxEnabled()) return; const x = this.game.x(unit.lastTile()); const y = this.game.y(unit.lastTile()); // ... rest of visual FX🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/FxLayer.ts` around lines 185 - 207, onWarshipEvent currently only plays visual FX when fxEnabled() is true and therefore misses the required warship destruction audio; move or add a call to SoundManager.playSoundEffect(SoundEffect.WarshipLost) so it executes whenever a UnitView is inactive (i.e., inside the outer if (!unit.isActive()) block) but outside the fxEnabled() guard that surrounds visual effects (refer to the onWarshipEvent method, UnitView.isActive(), fxEnabled(), and use SoundManager.playSoundEffect with SoundEffect.WarshipLost).
🧹 Nitpick comments (1)
src/client/graphics/layers/FxLayer.ts (1)
29-29: Consider periodic cleanup ofconstructionStateMap.This Map tracks construction state per unit ID. Entries are removed when units become inactive (line 222), but if a unit is ever removed from the game without triggering
onStructureEventin inactive state, the entry would persist. For a long-running game, this could accumulate stale entries.A simple fix: periodically prune entries for unit IDs that no longer exist in the game, or clear the Map on game reset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/FxLayer.ts` at line 29, The constructionState Map can accumulate stale entries; add a periodic prune and game-reset clear: implement a private method (e.g., pruneConstructionState) that iterates constructionState.keys(), checks unit existence via the game's unit lookup (e.g., getUnitById or units collection) and deletes entries for missing IDs, and call this method from a timer (or the main update loop) and from the game reset handler (e.g., onGameReset or similar); ensure calls are added alongside existing onStructureEvent and the Map is cleared during reset to prevent long-term growth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/EventsDisplay.ts`:
- Around line 553-558: The AllianceBroken sound is triggered before checking
whether the betrayed player is disconnected, which can cause the sound to play
while the UI event is skipped; in EventsDisplay.ts, move the
betrayed.isDisconnected() check to run before calling
SoundManager.playSoundEffect(SoundEffect.AllianceBroken) (and return early if
disconnected) so the sound is only played when the code will also add the
corresponding UI event for update where update.betrayedID or update.traitorID
equals myPlayer.smallID().
In `@src/client/graphics/layers/FxLayer.ts`:
- Around line 209-222: When a structure is detected destroyed in
onStructureEvent (if (!unit.isActive())), add a call to
SoundManager.playSoundEffect(SoundEffect.WarshipShot) so the warship-shot.mp3
plays on destruction; place this call inside the same if-block handling
destruction (near where SpriteFx/FxType.BuildingExplosion is created and before
constructionState.delete(unit.id())) so the sound always plays when a UnitView
becomes inactive.
- Around line 282-296: In handleSAMInterception, play the SAM interception sound
unconditionally by invoking the same sound playback used in handleNukeExplosion
to play SoundEffect.SAMHit (sam-hit.mp3) outside the fxEnabled() guard; locate
handleSAMInterception and add a call to the existing sound playback API (the
method used for nuke sound playback) before or after the fxEnabled() block so
the visual FX remain gated but the SoundEffect.SAMHit is always played.
---
Outside diff comments:
In `@src/client/graphics/layers/FxLayer.ts`:
- Around line 120-134: The onShellEvent handler currently creates a
MiniExplosion SpriteFx when a UnitView shell reaches its target but doesn't play
the impact sound; update onShellEvent so that when unit.reachedTarget() &&
this.fxEnabled() succeeds you also call
SoundManager.playSoundEffect(SoundEffect.WarshipShot) (alongside creating the
SpriteFx and pushing to this.allFx) to trigger the warship shell impact audio;
ensure you reference the existing SoundManager and SoundEffect enums when adding
this call.
- Around line 185-207: onWarshipEvent currently only plays visual FX when
fxEnabled() is true and therefore misses the required warship destruction audio;
move or add a call to SoundManager.playSoundEffect(SoundEffect.WarshipLost) so
it executes whenever a UnitView is inactive (i.e., inside the outer if
(!unit.isActive()) block) but outside the fxEnabled() guard that surrounds
visual effects (refer to the onWarshipEvent method, UnitView.isActive(),
fxEnabled(), and use SoundManager.playSoundEffect with SoundEffect.WarshipLost).
---
Nitpick comments:
In `@src/client/graphics/layers/FxLayer.ts`:
- Line 29: The constructionState Map can accumulate stale entries; add a
periodic prune and game-reset clear: implement a private method (e.g.,
pruneConstructionState) that iterates constructionState.keys(), checks unit
existence via the game's unit lookup (e.g., getUnitById or units collection) and
deletes entries for missing IDs, and call this method from a timer (or the main
update loop) and from the game reset handler (e.g., onGameReset or similar);
ensure calls are added alongside existing onStructureEvent and the Map is
cleared during reset to prevent long-term growth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2973b44-a251-408e-8b2e-f5e9d872492a
📒 Files selected for processing (4)
src/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/FxLayer.tssrc/client/graphics/layers/RadialMenu.tssrc/client/sound/SoundManager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/sound/SoundManager.ts
Comment on lines
209
to
+222
| onStructureEvent(unit: UnitView) { | ||
| if (!unit.isActive()) { | ||
| const x = this.game.x(unit.lastTile()); | ||
| const y = this.game.y(unit.lastTile()); | ||
| const explosion = new SpriteFx( | ||
| this.animatedSpriteLoader, | ||
| x, | ||
| y, | ||
| FxType.BuildingExplosion, | ||
| ); | ||
| this.allFx.push(explosion); | ||
| if (this.fxEnabled()) { | ||
| const x = this.game.x(unit.lastTile()); | ||
| const y = this.game.y(unit.lastTile()); | ||
| const explosion = new SpriteFx( | ||
| this.animatedSpriteLoader, | ||
| x, | ||
| y, | ||
| FxType.BuildingExplosion, | ||
| ); | ||
| this.allFx.push(explosion); | ||
| } | ||
| this.constructionState.delete(unit.id()); |
Contributor
There was a problem hiding this comment.
Missing structure destruction sound effect.
Per PR objectives, structure destruction should play warship-shot.mp3. Add SoundManager.playSoundEffect(SoundEffect.WarshipShot) when a structure is destroyed.
Proposed fix
onStructureEvent(unit: UnitView) {
if (!unit.isActive()) {
+ SoundManager.playSoundEffect(SoundEffect.WarshipShot);
if (this.fxEnabled()) {
const x = this.game.x(unit.lastTile());
// ... rest of visual FX🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/FxLayer.ts` around lines 209 - 222, When a
structure is detected destroyed in onStructureEvent (if (!unit.isActive())), add
a call to SoundManager.playSoundEffect(SoundEffect.WarshipShot) so the
warship-shot.mp3 plays on destruction; place this call inside the same if-block
handling destruction (near where SpriteFx/FxType.BuildingExplosion is created
and before constructionState.delete(unit.id())) so the sound always plays when a
UnitView becomes inactive.
Author
There was a problem hiding this comment.
warship-shot is "Waiting for Approval" in the sound spreadsheet and doesn't have an approved sound file yet. Will wire up once approved.
Contributor
There was a problem hiding this comment.
@camclark, understood! The PR objectives already document this clearly — warship-shot (along with silo-built, sam-shoot, sam-hit, and warship-lost) are intentionally deferred pending sound asset approval. I'll note this for future reviews.
✏️ Learnings added
Learnt from: camclark
Repo: openfrontio/OpenFrontIO PR: 3394
File: src/client/graphics/layers/FxLayer.ts:209-222
Timestamp: 2026-03-10T13:23:16.697Z
Learning: In OpenFrontIO (PR `#3394`, src/client/graphics/layers/FxLayer.ts), the following sound effects are intentionally NOT wired up yet because they are pending approval in the project's sound spreadsheet: silo-built, sam-shoot, sam-hit, warship-lost, warship-shot. Do not flag the absence of these sounds as issues. Wire them up only after they receive approval and have confirmed audio assets.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: wozniakpl
Repo: openfrontio/OpenFrontIO PR: 3317
File: src/client/InputHandler.ts:344-350
Timestamp: 2026-03-07T11:35:24.071Z
Learning: Ensure ghost preservation logic for special units (AtomBomb and HydrogenBomb) is implemented via shouldPreserveGhostAfterBuild(unitType) and that ghost removal is conditional on !shouldPreserveGhostAfterBuild(unitType) within createStructure. Confirm that both mouse (MouseUpEvent) and keyboard (ConfirmGhostStructureEvent) confirmation paths go through requestConfirmStructure → createStructure, and that Enter/NumpadEnter confirmations do not break the locked-nuke multi-placement since the ghost should be preserved by design. Add tests to cover shouldPreserveGhostAfterBuild returning true for AtomBomb/HydrogenBomb and verify that ghost removal only occurs for other units. Applies to files in the repository OpenFrontIO (src/client/InputHandler.ts and src/client/graphics/layers/StructureIconsLayer.ts).
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:30.357Z
Learning: In `src/client/graphics/layers/StructureIconsLayer.ts`, the confirm button in `ensureGhostControls()` for locked ghost nukes (AtomBomb/HydrogenBomb) intentionally does NOT call `removeGhostStructure()` after emitting the build intent. This allows mobile players to rapidly place multiple nukes by "spamming" the confirm button without needing to reopen the radial menu.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 0
File: :0-0
Timestamp: 2026-02-25T21:35:31.614Z
Learning: In NationExecution (src/core/execution/NationExecution.ts), even though it's active during spawn phase via activeDuringSpawnPhase() => true, it never calls handleStructures() during that phase - it only handles player spawning. The structure building code path is unreachable during spawn phase, making spawn immunity checks for structure spawning unnecessary.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 3337
File: src/client/graphics/layers/UnitLayer.ts:293-300
Timestamp: 2026-03-03T15:02:02.735Z
Learning: In UnitLayer.ts, unit sprites are square by design (sprite.width === sprite.height). Rendering optimizations that assume non-square sprites or attempt to fix aspect ratio issues do not apply to unit sprites in this layer.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 3246
File: src/core/execution/nation/NationNukeBehavior.ts:814-832
Timestamp: 2026-02-19T18:07:39.005Z
Learning: In src/core/execution/nation/NationNukeBehavior.ts, the clustering logic in maybeDestroyEnemySam() intentionally checks the arrival time of the first bomb from a silo rather than the last when evaluating if a silo fits within maxTotalArrivalSpread. This slight underestimate is by design: the window is already conservative (half the SAM cooldown = 37 of 75 ticks), staggerInterval is small (1-5 ticks), extra bombs provide margin, and the approach includes borderline silos that work fine in practice. The design should not be "fixed" to use the last bomb's arrival unless SAM timing changes significantly.
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.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Learnt from: evanpelle
Repo: openfrontio/OpenFrontIO PR: 3114
File: src/client/graphics/layers/AttacksDisplay.ts:64-77
Timestamp: 2026-02-10T05:02:28.715Z
Learning: In the OpenFrontIO codebase, GameUpdateType maps always have an array value for each key (never undefined). When iterating, you can safely use: for (const event of updates[GameUpdateType.X]) without optional chaining or guards. Treat updates as a Record<GameUpdateType, any[]> and rely on its guaranteed array values. If you modify the type, ensure the guarantee remains (e.g., initialize missing keys to []) to preserve safe iteration.
Comment on lines
282
to
296
| handleSAMInterception(unit: UnitView) { | ||
| const x = this.game.x(unit.lastTile()); | ||
| const y = this.game.y(unit.lastTile()); | ||
| const explosion = new SpriteFx( | ||
| this.animatedSpriteLoader, | ||
| x, | ||
| y, | ||
| FxType.SAMExplosion, | ||
| ); | ||
| this.allFx.push(explosion); | ||
| const shockwave = new ShockwaveFx(x, y, 800, 40); | ||
| this.allFx.push(shockwave); | ||
| if (this.fxEnabled()) { | ||
| const x = this.game.x(unit.lastTile()); | ||
| const y = this.game.y(unit.lastTile()); | ||
| const explosion = new SpriteFx( | ||
| this.animatedSpriteLoader, | ||
| x, | ||
| y, | ||
| FxType.SAMExplosion, | ||
| ); | ||
| this.allFx.push(explosion); | ||
| const shockwave = new ShockwaveFx(x, y, 800, 40); | ||
| this.allFx.push(shockwave); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing SAM interception sound effect.
Per PR objectives, SAM interception should play sam-hit.mp3 via SoundEffect.SAMHit. The visual FX is gated behind fxEnabled(), but no sound is played at all. Add sound playback outside the fxEnabled() guard to match the pattern used in handleNukeExplosion.
Proposed fix
handleSAMInterception(unit: UnitView) {
if (this.fxEnabled()) {
const x = this.game.x(unit.lastTile());
const y = this.game.y(unit.lastTile());
const explosion = new SpriteFx(
this.animatedSpriteLoader,
x,
y,
FxType.SAMExplosion,
);
this.allFx.push(explosion);
const shockwave = new ShockwaveFx(x, y, 800, 40);
this.allFx.push(shockwave);
}
+ SoundManager.playSoundEffect(SoundEffect.SAMHit);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/FxLayer.ts` around lines 282 - 296, In
handleSAMInterception, play the SAM interception sound unconditionally by
invoking the same sound playback used in handleNukeExplosion to play
SoundEffect.SAMHit (sam-hit.mp3) outside the fxEnabled() guard; locate
handleSAMInterception and add a call to the existing sound playback API (the
method used for nuke sound playback) before or after the fxEnabled() block so
the visual FX remain gated but the SoundEffect.SAMHit is always played.
Author
There was a problem hiding this comment.
sam-hit is "Waiting for Approval" in the sound spreadsheet — no approved sound file exists yet. Will wire up once approved.
Contributor
There was a problem hiding this comment.
@camclark, understood — thanks for the clarification! Since sam-hit is still waiting approval in the sound spreadsheet, skipping it for now makes total sense. The five unapproved assets (silo-built, sam-shoot, sam-hit, warship-lost, warship-shot) are intentionally excluded until the sounds are approved. I'll disregard this comment.
(´• ω •`) 🐇
🧠 Learnings used
Learnt from: wozniakpl
Repo: openfrontio/OpenFrontIO PR: 3317
File: src/client/InputHandler.ts:344-350
Timestamp: 2026-03-07T11:35:24.071Z
Learning: Ensure ghost preservation logic for special units (AtomBomb and HydrogenBomb) is implemented via shouldPreserveGhostAfterBuild(unitType) and that ghost removal is conditional on !shouldPreserveGhostAfterBuild(unitType) within createStructure. Confirm that both mouse (MouseUpEvent) and keyboard (ConfirmGhostStructureEvent) confirmation paths go through requestConfirmStructure → createStructure, and that Enter/NumpadEnter confirmations do not break the locked-nuke multi-placement since the ghost should be preserved by design. Add tests to cover shouldPreserveGhostAfterBuild returning true for AtomBomb/HydrogenBomb and verify that ghost removal only occurs for other units. Applies to files in the repository OpenFrontIO (src/client/InputHandler.ts and src/client/graphics/layers/StructureIconsLayer.ts).
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:30.357Z
Learning: In `src/client/graphics/layers/StructureIconsLayer.ts`, the confirm button in `ensureGhostControls()` for locked ghost nukes (AtomBomb/HydrogenBomb) intentionally does NOT call `removeGhostStructure()` after emitting the build intent. This allows mobile players to rapidly place multiple nukes by "spamming" the confirm button without needing to reopen the radial menu.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 3246
File: src/core/execution/nation/NationNukeBehavior.ts:814-832
Timestamp: 2026-02-19T18:07:39.005Z
Learning: In src/core/execution/nation/NationNukeBehavior.ts, the clustering logic in maybeDestroyEnemySam() intentionally checks the arrival time of the first bomb from a silo rather than the last when evaluating if a silo fits within maxTotalArrivalSpread. This slight underestimate is by design: the window is already conservative (half the SAM cooldown = 37 of 75 ticks), staggerInterval is small (1-5 ticks), extra bombs provide margin, and the approach includes borderline silos that work fine in practice. The design should not be "fixed" to use the last bomb's arrival unless SAM timing changes significantly.
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 3337
File: src/client/graphics/layers/UnitLayer.ts:293-300
Timestamp: 2026-03-03T15:02:02.735Z
Learning: In UnitLayer.ts, unit sprites are square by design (sprite.width === sprite.height). Rendering optimizations that assume non-square sprites or attempt to fix aspect ratio issues do not apply to unit sprites in this layer.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 0
File: :0-0
Timestamp: 2026-02-01T16:36:52.217Z
Learning: In this TypeScript game codebase, UI/layer updates should be wall-clock throttled (independent of simulation speed). Do not use tick-modulus gating like `if (game.ticks()%N===0)` inside `layer.tick()` methods. Instead, layers should expose a `getTickIntervalMs()` method and let the renderer schedule ticks based on wall-clock time.
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
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.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Learnt from: evanpelle
Repo: openfrontio/OpenFrontIO PR: 3114
File: src/client/graphics/layers/AttacksDisplay.ts:64-77
Timestamp: 2026-02-10T05:02:28.715Z
Learning: In the OpenFrontIO codebase, GameUpdateType maps always have an array value for each key (never undefined). When iterating, you can safely use: for (const event of updates[GameUpdateType.X]) without optional chaining or guards. Treat updates as a Record<GameUpdateType, any[]> and rely on its guaranteed array values. If you modify the type, ensure the guarantee remains (e.g., initialize missing keys to []) to preserve safe iteration.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/EventsDisplay.ts`:
- Around line 558-563: The code currently calls
SoundManager.playSoundEffect(SoundEffect.AllianceBroken) based solely on
update.betrayedID/update.traitorID matching myPlayer.smallID(), which can play
audio even when no event is added; remove that standalone playback and instead
invoke SoundManager.playSoundEffect(SoundEffect.AllianceBroken) inside the two
branches where addEvent(...) is called (the branches that create the "betrayed"
and "traitor" events) so the sound only plays when addEvent actually runs; keep
references to update.betrayedID, update.traitorID, myPlayer.smallID(), addEvent,
SoundManager.playSoundEffect and SoundEffect.AllianceBroken to locate the
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d7cc377-63a7-456f-b5ad-ca43acee44bf
📒 Files selected for processing (1)
src/client/graphics/layers/EventsDisplay.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Adds sound effects for all approved events from the sound asset pack. Extends
SoundManagerwith 15 newSoundEffectenum values and hooks sound triggers intoFxLayer,EventsDisplay, andRadialMenu. Sound playback is decoupled from the visual FX toggle — audio plays even when FX layer is disabled. Only approved sounds from the spreadsheet are wired up; sounds marked "Waiting for Approval" are included as assets but not played.Sound approval status (per spreadsheet)
Approved — wired up in this PR
message.mp3click.mp3atom-launch.mp3atom-hit.mp3hydrogen-launch.mp3hydrogen-hit.mp3mirv-launch.mp3alliance-suggested.mp3alliance-broken.mp3build-port.mp3build-city.mp3build-defense-post.mp3build-warship.mp3sam-built.mp3Waiting for Approval — sound files included but NOT wired up
silo-built.mp3sam-shoot.mp3Not done — no sound files exist yet
Test plan
Checklist
Resolves #1893
Please put your Discord username so you can be contacted if a bug or regression is found:
cool_clarky