Group event notifications and inbound nukes in quick succession#4530
Group event notifications and inbound nukes in quick succession#4530blontd6 wants to merge 1 commit into
Conversation
|
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 (3)
💤 Files with no reviewable changes (3)
WalkthroughAdds grouping and pluralization for repeated event notifications (destroyed, captured, lost, inbound nukes) in EventsDisplay, with new localization strings. UnitImpl now emits capture/loss display messages for structures and widens the delete-message suppression filter to include structures. ChangesEvent grouping feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UnitImpl
participant EventsDisplay
participant AddEvent as addEvent()
participant UI as Event Log UI
UnitImpl->>EventsDisplay: emit unit_captured / unit_lost / unit_destroyed
EventsDisplay->>EventsDisplay: derive groupKey, unitType, targetPlayerName
EventsDisplay->>AddEvent: pass event with groupKey
alt matching groupKey within 30 ticks
AddEvent->>AddEvent: increment count, refresh createdAt, regenerate description
else no match
AddEvent->>AddEvent: append new event row
end
AddEvent->>UI: render grouped/pluralized message
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ 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: 1
🧹 Nitpick comments (1)
src/client/hud/layers/EventsDisplay.ts (1)
37-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile string parsing with magic offsets.
parseInboundNukereverse-engineers the player name from hardcoded lengths (-20,-24,7,-22) of the exact English message. If any of these strings change (or the message is localized), the slice silently returns the wrong name ornull, and grouping quietly stops working. The MIRV offsets are especially error-prone because each⚠️is 2 UTF-16 units.Cleaner option: pass the nuke type and player name as structured fields on the incoming event so no string parsing is needed. If parsing must stay, at least compute offsets from the suffix constants instead of literals:
♻️ Derive offsets from the suffix strings
function parseInboundNuke(m: string): { player: string; nukeType: string } | null { - if (m.endsWith(" - atom bomb inbound")) return { player: m.slice(0, -20), nukeType: "atom" }; - if (m.endsWith(" - hydrogen bomb inbound")) return { player: m.slice(0, -24), nukeType: "hydrogen" }; - if (m.startsWith("⚠️⚠️⚠️ ") && m.endsWith(" - MIRV INBOUND ⚠️⚠️⚠️")) return { player: m.slice(7, -22), nukeType: "mirv" }; + const atom = " - atom bomb inbound"; + const hydrogen = " - hydrogen bomb inbound"; + const mirvPre = "⚠️⚠️⚠️ "; + const mirvSuf = " - MIRV INBOUND ⚠️⚠️⚠️"; + if (m.endsWith(atom)) return { player: m.slice(0, -atom.length), nukeType: "atom" }; + if (m.endsWith(hydrogen)) return { player: m.slice(0, -hydrogen.length), nukeType: "hydrogen" }; + if (m.startsWith(mirvPre) && m.endsWith(mirvSuf)) return { player: m.slice(mirvPre.length, -mirvSuf.length), nukeType: "mirv" }; return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/hud/layers/EventsDisplay.ts` around lines 37 - 42, The parseInboundNuke helper is relying on fragile hardcoded slice offsets to recover the player name from event text. Update EventsDisplay.parseInboundNuke to avoid magic numbers by either passing structured player/nuke fields into the event handling path, or by deriving the start/end offsets from the exact suffix strings used in each message pattern. Keep the existing function and its MIRV branch, but make the parsing length-safe so changes to the message text or Unicode characters do not break grouping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/hud/layers/EventsDisplay.ts`:
- Line 35: The pluralizeUnit helper in EventsDisplay hardcodes English plural
forms, so update it to use the translated unit-name keys from the language
resources instead of deriving text from the raw enum string. Locate the
pluralization logic in pluralizeUnit and switch it to resolve the appropriate
unit.* entries from the i18n layer so event text stays locale-safe.
---
Nitpick comments:
In `@src/client/hud/layers/EventsDisplay.ts`:
- Around line 37-42: The parseInboundNuke helper is relying on fragile hardcoded
slice offsets to recover the player name from event text. Update
EventsDisplay.parseInboundNuke to avoid magic numbers by either passing
structured player/nuke fields into the event handling path, or by deriving the
start/end offsets from the exact suffix strings used in each message pattern.
Keep the existing function and its MIRV branch, but make the parsing length-safe
so changes to the message text or Unicode characters do not break grouping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24d4b3ef-03e5-4da4-a2fa-e002f60f946c
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/hud/layers/EventsDisplay.tssrc/core/game/UnitImpl.ts
| translateText, | ||
| } from "../../Utils"; | ||
|
|
||
| const pluralizeUnit = (u: string) => u === "City" ? "Cities" : u === "Factory" ? "Factories" : u + "s"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for existing translation keys for unit type names.
rg -n --type=json -i '"(city|cities|factory|factories|port|silo|defense)"' resources/lang/en.jsonRepository: openfrontio/OpenFrontIO
Length of output: 644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby translation usage.
git ls-files 'src/client/hud/layers/EventsDisplay.ts' 'src/client/**/*.{ts,tsx}' 'resources/lang/en.json' | sed -n '1,20p'
echo '--- EventsDisplay.ts ---'
sed -n '1,220p' src/client/hud/layers/EventsDisplay.ts
echo '--- search for pluralizeUnit / translateText / unit type translation keys ---'
rg -n 'pluralizeUnit|translateText|build_city|build_factory|cities|factories|UnitType|unit:' src/client resources/lang/en.jsonRepository: openfrontio/OpenFrontIO
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/client/hud/layers/EventsDisplay.ts
echo '---'
rg -n 'pluralizeUnit|translateText|build_city|build_factory|cities|factories|UnitType|unit:' src/client resources/lang/en.jsonRepository: openfrontio/OpenFrontIO
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- EventsDisplay.ts ---'
cat -n src/client/hud/layers/EventsDisplay.ts | sed -n '1,220p'
echo '--- translation-related matches ---'
rg -n 'pluralizeUnit|translateText|build_city|build_factory|cities|factories|UnitType|unit:' src/client resources/lang/en.jsonRepository: openfrontio/OpenFrontIO
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- pluralizeUnit usage in EventsDisplay.ts ---'
rg -n 'pluralizeUnit' src/client/hud/layers/EventsDisplay.ts
echo '--- unit_type keys in resources/lang/en.json ---'
rg -n '"unit_type\.' resources/lang/en.json
echo '--- nearby English unit names in resources/lang/en.json ---'
sed -n '1150,1175p' resources/lang/en.json
sed -n '1395,1415p' resources/lang/en.json
echo '--- EventsDisplay.ts around all unit-related formatting ---'
rg -n -A3 -B3 'unitType|pluralizeUnit|translateText\(' src/client/hud/layers/EventsDisplay.tsRepository: openfrontio/OpenFrontIO
Length of output: 427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- EventsDisplay.ts around the pluralized unit text ---'
sed -n '250,290p' src/client/hud/layers/EventsDisplay.ts
echo '--- unit_type keys in resources/lang/en.json ---'
rg -n '"unit_type\.(city|factory|port|defense_post|warship|boat|missile_silo|sam_launcher|atom_bomb|hydrogen_bomb|mirv)"' resources/lang/en.json
echo '--- nearby unit_type section ---'
sed -n '1398,1420p' resources/lang/en.jsonRepository: openfrontio/OpenFrontIO
Length of output: 1978
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- resources/lang/en.json around the unit type strings ---'
sed -n '1156,1172p' resources/lang/en.json
echo '--- EventsDisplay.ts around the pluralized unit text ---'
sed -n '268,282p' src/client/hud/layers/EventsDisplay.tsRepository: openfrontio/OpenFrontIO
Length of output: 1479
Use translated unit names here pluralizeUnit hardcodes English forms (Cities, Factories, + "s"). The resources/lang/en.json file already has unit.* keys, so this should use those keys instead of raw enum strings to keep the event text locale-safe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/hud/layers/EventsDisplay.ts` at line 35, The pluralizeUnit helper
in EventsDisplay hardcodes English plural forms, so update it to use the
translated unit-name keys from the language resources instead of deriving text
from the raw enum string. Locate the pluralization logic in pluralizeUnit and
switch it to resolve the appropriate unit.* entries from the i18n layer so event
text stays locale-safe.
Source: Coding guidelines
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
blontd6
force-pushed
the
feature/group-event-messages
branch
from
f25a559 to
e2fd466
Compare
1 participant
Add approved & assigned issue number here:
Resolves #4516
Description:
Restores real-time event notifications for structure capture, loss, and destruction (such as Cities, Ports, Silos, Factories, SAM Launchers, and Defense Posts), and aggregates these notifications alongside inbound atomic bombs, hydrogen bombs, and MIRVs when they occur in quick succession (within a 3-second / 30-tick window).
formats such as:
"Your 4 Cities were destroyed""Captured 4 Cities from [Player]""[Player] - 3 atom bombs inbound"This provides clear, concise, and immediate tactical feedback to players in high-stress combat moments without sacrificing important event tracking.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
blontd6