Draft
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded two localization keys; changed nuke detonation to include SAM missiles; added a SAM refire-delay config and logic that delays retargeting, clears target SAM flags, and sends defender/attacker notifications when an interceptor is destroyed. Changes
Sequence DiagramsequenceDiagram
participant Attacker
participant Nuke
participant SAM as SAM_Interceptor
participant Target
participant Defender
Attacker->>Nuke: Launch nuke at Target
Defender->>SAM: Deploy interceptor toward Nuke
SAM->>Nuke: Attempt interception
Nuke->>SAM: Explosion / blast affects SAM
alt SAM destroyed and Target still active
SAM->>SAM: Start/consume refireDelayRemaining ticks
SAM->>Target: (on delay expiry) Clear SAM-target flag
SAM-->>Defender: Notify "Interceptor destroyed, retargeting {unit}"
SAM-->>Attacker: Notify "{unit} survived interception, will re-attempt interception, if possible"
SAM-->>SAM: Mark inactive / stop interception
else SAM destroyed and Target gone
SAM-->>SAM: Mark inactive / stop interception
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🧹 Nitpick comments (1)
src/core/execution/SAMMissileExecution.ts (1)
42-61: Please add a regression test for the interceptor-destroyed branch.This branch now changes targeting state and emits two player-facing messages. A focused test would protect against future regressions in refire flow and event text routing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/SAMMissileExecution.ts` around lines 42 - 61, Add a regression test covering the interceptor-destroyed branch in SAMMissileExecution: simulate an interceptor being destroyed so this.target.isActive() is true, run the SAMMissileExecution flow, assert that the target's setTargetedBySAM(false) behaviour occurs (target no longer flagged as targeted by SAM) and verify mg.displayMessage was called twice with the expected keys ("events_display.interceptor_destroyed" and "events_display.interceptor_destroyed_attacker") and correct MessageType and player ids (this._owner.id() and this.target.owner().id()); use spies/mocks for mg.displayMessage and the target owner/id methods to validate messages and state changes to prevent regressions in refire flow and event routing.
🤖 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/core/execution/SAMMissileExecution.ts`:
- Around line 54-60: The message emitted in SAMMissileExecution via
this.mg.displayMessage using key events_display.interceptor_destroyed_attacker
and MessageType.SAM_MISS implies a new interceptor is inbound even though the
code only clears the SAM-target flag; update the logic to avoid promising a
refire by either (a) changing the message key/text to neutral wording (e.g.,
"interceptor_destroyed" or "sam_target_cleared") in the same display call, or
(b) perform a real availability check on the attacker's launchers (use the
owner's launcher/launcherPool APIs or a canFire()/availableLauncher() check on
this.target.owner()) and only call this.mg.displayMessage with the existing key
when an actual interceptor launch is confirmed; modify SAMMissileExecution
accordingly to reference events_display.interceptor_destroyed_attacker,
this.mg.displayMessage, and this.target.owner().
---
Nitpick comments:
In `@src/core/execution/SAMMissileExecution.ts`:
- Around line 42-61: Add a regression test covering the interceptor-destroyed
branch in SAMMissileExecution: simulate an interceptor being destroyed so
this.target.isActive() is true, run the SAMMissileExecution flow, assert that
the target's setTargetedBySAM(false) behaviour occurs (target no longer flagged
as targeted by SAM) and verify mg.displayMessage was called twice with the
expected keys ("events_display.interceptor_destroyed" and
"events_display.interceptor_destroyed_attacker") and correct MessageType and
player ids (this._owner.id() and this.target.owner().id()); use spies/mocks for
mg.displayMessage and the target owner/id methods to validate messages and state
changes to prevent regressions in refire flow and event routing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a5438c7-aa08-4308-b36e-e2b1b60ef60b
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/core/execution/NukeExecution.tssrc/core/execution/SAMMissileExecution.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
…rceptor, added communication to attk/defender that interceptors have been destroyed.
Caleb-Cohen
force-pushed
the
test/sam-refire
branch
from
717d3e4 to
5a19aa8
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/execution/SAMMissileExecution.ts (1)
43-67: Add focused regression tests for this timing branch.This block is stateful and time-based; please add tests that verify delay countdown, single flag clear, and one-time message emission.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/SAMMissileExecution.ts` around lines 43 - 67, The timing branch in SAMMissileExecution that uses refireDelayRemaining/refireDelayRemaining ??= and handles target.isActive(), target.setTargetedBySAM(false), and mg.displayMessage(...) needs focused regression tests: add tests that (1) initialize a SAMMissileExecution with a live interceptor and verify refireDelayRemaining is set to mg.config().samRefireDelayTicks() and counts down each tick until zero, (2) assert target.setTargetedBySAM(false) is called exactly once when the delay expires (not on intermediate ticks), and (3) assert mg.displayMessage is invoked once for both "events_display.interceptor_destroyed" and "events_display.interceptor_destroyed_attacker" only when the delay reaches zero; use or mock methods isActive(), owner().id(), and config().samRefireDelayTicks() to control timing and observe side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/execution/SAMMissileExecution.ts`:
- Around line 43-67: The timing branch in SAMMissileExecution that uses
refireDelayRemaining/refireDelayRemaining ??= and handles target.isActive(),
target.setTargetedBySAM(false), and mg.displayMessage(...) needs focused
regression tests: add tests that (1) initialize a SAMMissileExecution with a
live interceptor and verify refireDelayRemaining is set to
mg.config().samRefireDelayTicks() and counts down each tick until zero, (2)
assert target.setTargetedBySAM(false) is called exactly once when the delay
expires (not on intermediate ticks), and (3) assert mg.displayMessage is invoked
once for both "events_display.interceptor_destroyed" and
"events_display.interceptor_destroyed_attacker" only when the delay reaches
zero; use or mock methods isActive(), owner().id(), and
config().samRefireDelayTicks() to control timing and observe side effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d10e17b5-1455-46e9-82ab-4a9a838c0e24
📒 Files selected for processing (3)
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/SAMMissileExecution.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.
Requested by Evan in Discord to review: here
Since this bypasses issues, I'm looking for a confirmation from a maintainer before I finish this PR. Its current state is just meant to demonstrate the MVP.
Description:
#3167 added SAM missile immunity to counter nuke/hydro & hydro/hydro strategies. As a result, large trade-maxing games can result in lengthy stalemates as clusters of high-level SAMs can be almost impenetrable.
#3223 added SAM missile re-targeting so SAMs will re-engage atomics whose initial interceptors were destroyed. This PR did not remove the invulnerability on the SAM interceptor.
This PR removes SAM interceptor invulnerability and adds communication to the attacker and defender that SAM interceptors have been destroyed and re-attempts.
This change will make interceptor destroying a viable strategy again, but it will be significantly nerfed compared to pre #3167 since there was a refire mechanic implemented in #3223.
Attached is a video showing the mechanic. First attack shows SAM engaging correctly on initial contact, second attack shows SAM re-engaging once the first interceptor was destroyed.
re-target_communication.1.mp4
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
calebcohen