Skip to content

Conversation

@kerodkibatu
Copy link
Contributor

@kerodkibatu kerodkibatu commented Nov 1, 2025

Description:

Added prominent red alert notifications for incoming land attacks, reusing the existing betrayal alert mechanism. Players will now receive visual feedback when they are being attacked, improving awareness of incoming threats.

Addressing #2355

Please complete the following:

  • I have added screenshots for all UI updates
image
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

kerverse

…on player status. Added functionality to clear seen attack IDs when the player dies and to check for new attacks, ensuring alerts are only activated for non-retreating attacks. This improves the game's responsiveness to player actions and incoming threats.
@kerodkibatu kerodkibatu requested a review from a team as a code owner November 1, 2025 07:30
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 1, 2025

Walkthrough

Tracks and records recent outgoing attacks and incoming attacks in AlertFrame, adds retaliation window and alert cooldown constants, prevents duplicate alerts with a seen set, prunes stale tracking state when player is missing/dead, and updates lastAlertTick when an alert is activated.

Changes

Cohort / File(s) Change Summary
Alert frame logic
src/client/graphics/layers/AlertFrame.ts
Added RETALIATION_WINDOW_TICKS and ALERT_COOLDOWN_TICKS; introduced private fields seenAttackIds, lastAlertTick, outgoingAttackTicks; refactored tick() to clear state when player missing/dead, call trackOutgoingAttacks() and checkForNewAttacks(); implemented trackOutgoingAttacks() to record/prune recent outgoing attacks; implemented checkForNewAttacks() to ignore retaliations, filter small attacks, respect alert cooldown, mark seen IDs, activate alerts, and clean stale seen IDs; activateAlert() now sets lastAlertTick. No public API changes.

Sequence Diagram(s)

sequenceDiagram
    participant Game as Game.ticks()
    participant Alert as AlertFrame
    participant Out as trackOutgoingAttacks()
    participant In as checkForNewAttacks()
    participant Seen as seenAttackIds
    participant UI as activateAlert()

    Game->>Alert: tick()
    Alert->>Alert: is player alive?
    alt missing or dead
        Alert->>Seen: clear()
        Alert->>Out: clear()
        Alert-->>Game: return
    else alive
        Alert->>Out: record outgoing attack timestamps
        Out->>Out: prune older than RETALIATION_WINDOW_TICKS
        Alert->>In: iterate incomingAttacks
        loop each incoming attack
            In->>Seen: contains(id)?
            alt already seen
                In-->>Seen: skip
            else not seen
                In->>Out: check recent outgoing from same attacker (retaliation?)
                In->>Alert: is small attack? within cooldown?
                alt retaliation OR small OR cooldown
                    In->>Seen: add(id) // no alert
                else
                    In->>Seen: add(id)
                    In->>UI: activateAlert()
                    UI->>Alert: set lastAlertTick = Game.ticks()
                end
            end
        end
        In->>Seen: remove ids no longer active
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Focus areas:
    • Correctness of time-window pruning in outgoingAttackTicks.
    • Retaliation detection logic and tick comparisons.
    • Cooldown gating with lastAlertTick and interaction with activateAlert().
    • Proper cleanup when player is missing/dead and when attacks retreat.

Possibly related PRs

Suggested reviewers

  • evanpelle
  • scottanderson

Poem

⚔️ Outbound ticks logged into a line,
incoming strikes checked against time.
Retaliations hushed, small fights spared,
cooldown waits — fresh alerts declared.

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding visual alert notifications when the player is attacked.
Description check ✅ Passed The description clearly explains the feature added, references the related issue, and confirms completion of required tasks including testing and localization.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa6e7d5 and 5e06587.

📒 Files selected for processing (1)
  • src/client/graphics/layers/AlertFrame.ts (4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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:

  • src/client/graphics/layers/AlertFrame.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/graphics/layers/AlertFrame.ts
📚 Learning: 2025-10-18T17:54:01.311Z
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.

Applied to files:

  • src/client/graphics/layers/AlertFrame.ts
📚 Learning: 2025-08-24T11:13:08.495Z
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.

Applied to files:

  • src/client/graphics/layers/AlertFrame.ts
📚 Learning: 2025-10-27T09:47:26.395Z
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.

Applied to files:

  • src/client/graphics/layers/AlertFrame.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/AlertFrame.ts (5)
src/core/game/GameView.ts (1)
  • myPlayer (592-594)
src/core/game/GameUpdates.ts (1)
  • BrokeAllianceUpdate (197-201)
src/core/game/StatsImpl.ts (1)
  • attack (149-158)
src/client/graphics/GameRenderer.ts (1)
  • tick (395-397)
src/client/graphics/layers/EventsDisplay.ts (1)
  • tick (189-253)
🔇 Additional comments (6)
src/client/graphics/layers/AlertFrame.ts (6)

14-15: Constants address past feedback on timing.

The 15-second values for retaliation window and alert cooldown are reasonable and address the reviewer's concerns about alert frequency. These appear to be tuning parameters that may need adjustment based on playtesting feedback.


26-29: State management looks solid.

The tracking structures are well-chosen: Set for deduplication, Map for per-player timestamps, and -1 sentinel for uninitialized state.


85-93: Proper cleanup when player is missing or dead.

Clearing all tracking state when the player dies prevents stale data and ensures a clean slate when respawning.


134-167: Excellent fix for the retaliation window bug!

This implementation correctly addresses the critical issue raised in previous reviews. The timestamp is now only set when first seeing an attack or when the previous entry has expired (lines 152-156), allowing the retaliation window to actually expire. The cleanup logic ensures stale entries are removed.

Note: The >= on line 154 vs > on line 163 is intentional—when the difference equals exactly RETALIATION_WINDOW_TICKS, we allow updating the timestamp but don't delete it in the same tick, which is the correct behavior.


129-129: Cooldown timestamp correctly updated.

Setting lastAlertTick when the alert activates ensures the cooldown logic works properly.


169-222: The review comment is incorrect. Ignore the BigInt concern.

All troops values in the codebase are typed as number, not BigInt:

  • PlayerView.troops() returns number
  • Attack.troops() returns number
  • AttackUpdate.troops is number

The division on line 185 (playerTroops / 5) and comparison on line 197 (attack.troops < minAttackTroopsThreshold) work correctly. The code needs no changes.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/client/graphics/layers/AlertFrame.ts (1)

80-86: Consider simplifying duplicate validation.

The player existence and liveness check here is duplicated in checkForNewAttacks() (lines 124-127). Since the early return on line 86 guarantees that myPlayer exists and is alive when reaching line 96, the check inside checkForNewAttacks() is redundant.

You could remove lines 124-127 to simplify the code, though the duplicate check doesn't cause issues (defensive programming).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between daf3fee and cc41d97.

📒 Files selected for processing (1)
  • src/client/graphics/layers/AlertFrame.ts (3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-18T17:54:01.311Z
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.

Applied to files:

  • src/client/graphics/layers/AlertFrame.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/graphics/layers/AlertFrame.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/AlertFrame.ts (1)
src/core/game/GameView.ts (1)
  • myPlayer (592-594)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: 🔍 ESLint
🔇 Additional comments (3)
src/client/graphics/layers/AlertFrame.ts (3)

24-24: Good choice of data structure.

Using a Set for tracking unique attack IDs is clean and efficient for duplicate detection.


95-96: LGTM!

Clean integration of the new attack detection logic into the tick cycle.


123-149: ****

The incomingAttacks() method is fully typed to return Attack[] (see src/core/game/PlayerImpl.ts:1170 and src/core/game/Game.ts:651), never null or undefined. TypeScript's type system guarantees the return value is always an array. The for-loop on line 132 is safe and will not crash.

Likely an incorrect or invalid review comment.

coderabbitai[bot]
coderabbitai bot previously approved these changes Nov 1, 2025
@TheGiraffe3 TheGiraffe3 linked an issue Nov 1, 2025 that may be closed by this pull request
coderabbitai[bot]
coderabbitai bot previously approved these changes Nov 4, 2025
@kerodkibatu kerodkibatu requested a review from evanpelle November 5, 2025 18:39
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe98e93 and aa6e7d5.

📒 Files selected for processing (1)
  • src/client/graphics/layers/AlertFrame.ts (4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:

  • src/client/graphics/layers/AlertFrame.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/graphics/layers/AlertFrame.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/AlertFrame.ts (2)
src/core/game/GameView.ts (1)
  • myPlayer (592-594)
src/client/graphics/layers/EventsDisplay.ts (1)
  • tick (189-253)

Copy link
Collaborator

@evanpelle evanpelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@evanpelle evanpelle added this to the v27 milestone Nov 6, 2025
@evanpelle evanpelle merged commit 0204866 into openfrontio:main Nov 6, 2025
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better UI/notifications when attacked

2 participants