Group event notifications and inbound nukes in quick succession#4530
Group event notifications and inbound nukes in quick succession#4530blontd6 wants to merge 4 commits into
Conversation
|
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:
WalkthroughAdds pluralized localization for grouped event messages, groups repeated destroyed/captured/lost and inbound nuke notifications within a short window, and updates structure capture and delete notifications in unit logic. ChangesEvent grouping feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
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
|
looks like the CI is failing |
blontd6
force-pushed
the
feature/group-event-messages
branch
from
faac666 to
ab42d1e
Compare
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
CLAUDE:
Overview
Restores owner-facing notifications when structures (City, Port, Silo, Factory, SAM, Defense Post) are captured, lost, or destroyed, and aggregates repeated notifications of the same kind arriving within 30 ticks into a single counted entry ("Captured 4 Cities from X", "3 atom bombs inbound"). Changes land in three places: new en.json keys, grouping logic in EventsDisplay.ts, and message emission in core's UnitImpl.setOwner() / displayMessageOnDeleted().
The feature itself is sound: setOwner is only reached via captureUnit() (warship boarding, player elimination in PlayerExecution), the loser/winner attribution is correct since messages fire before this._owner is reassigned, and group keys are correctly scoped by unit type and counterparty so simultaneous captures from two players don't merge. I verified the emoji/suffix slice offsets in parseInboundNuke are correct in UTF-16. But there are several issues worth fixing before merge.
Blocking issues
- No tests, and core was changed. The PR checkbox claims tests were added, but the diff touches only 3 files — none are tests. Project rules require tests for all src/core changes. tests/util/Setup.ts makes this easy: capture a structure and assert the two DisplayMessageUpdates (key, params, recipient); nuke a structure and assert unit_destroyed.
- The _unused string-array hack in EventsDisplay.ts is the wrong mechanism. It exists to fool tests/TranslationSystem.test.ts's unused-key check, because translation keys are built by string concatenation ("events_display." + nukeType + "_bomb_inbound_plural"). That test already provides the sanctioned escape hatch — DYNAMIC_KEY_PATTERNS, documented as "keeps dynamic handling explicit and reviewable." Better still, eliminate the dynamic construction entirely with a static map ({ atom: "events_display.atom_bomb_inbound", ... }), which makes the keys statically discoverable and deletes 27 lines of dead code plus the eslint-disable.
- parseInboundNuke re-parses English display strings the core just built — and half of it is redundant. The UnitIncomingUpdate already carries messageType (NUKE_INBOUND, HYDROGEN_BOMB_INBOUND, MIRV_INBOUND), which identifies the nuke type without any string matching. Only the attacker name needs extracting, and since both sides of this update live in this repo, the clean fix is to have NukeExecution/MIRVExecution send the key + params (there are literal // TODO TranslateText comments at both call sites) or at least the attacker's name as a field. As written, the magic offsets (slice(0, -20), slice(7, -22)) silently stop matching if anyone touches the core string — grouping and translation degrade with no error.
Convention issues
- Plural handling contradicts the established ICU pattern. Two lines above the new keys, mirv_warheads_intercepted uses ICU plural syntax: {count, plural, one {...} other {...}}. This PR instead adds parallel _plural keys and picks between them in TypeScript. Languages with more than two plural forms (Russian, Polish, Arabic — all in Crowdin) cannot be translated correctly under the _plural scheme; ICU exists precisely for this. Folding each pair into one ICU key also removes the singular/plural branching in addEvent.
- Unrelated style churn. The rewrite of onDisplayMessageEvent and onUnitIncomingEvent strips braces from guard clauses and deletes the explanatory comment about captured trade-ship gold being surfaced as a control-panel pip. That comment carries real context; please restore it and keep the reformatting out of the diff.
Smaller points
- The grouping window is rolling, not fixed. Each merge resets existing.createdAt, so a sustained barrage extends one entry indefinitely (and keeps it past the 80-tick display expiry). That's arguably the right UX for "12 atom bombs inbound," but it doesn't match the PR description's "3-second window" — confirm it's intended.
- unit_destroyed_plural ("{count} {unit} destroyed") drops the "Your" that the singular and the PR description ("Your 4 Cities were destroyed") both have.
- if (event.focusID) existing.focusID = event.focusID — falsy check would skip a legitimate ID of 0; use !== undefined.
- event.unitType holds two vocabularies — raw UnitType strings ("City") for display messages, but "atom"/"hydrogen"/"mirv" for inbound — in one field. Works, but it's a trap for the next reader; a comment or separate field would help.
- In the intercepted_ merge branch, getTranslatedUnitName(...) is recomputed even though u above already holds the same value.
- New noise path: in PlayerExecution, a dead player's structures on unowned tiles hit u.delete() (message defaults on), so the eliminated player now receives "destroyed" notifications during their own elimination. Grouping caps the spam and the player is on the death screen, so likely acceptable — just noting it's new.
Security / performance
- Player display names are interpolated into descriptions rendered with unsafeDescription: true, but that's the pre-existing pattern for every other {name} message in this file — no new exposure.
- addEvent's linear find runs over a list capped at ~30 rendered events; negligible.
- Core changes only add displayMessage updates (no state divergence risk to determinism).
Verdict
Good feature, correct core attribution logic, and translating unit names via unit_type.* is a genuine improvement over the raw enum strings shown before. Request changes for: missing core tests, the _unused hack (use DYNAMIC_KEY_PATTERNS or static key maps), string-parsing inbound messages instead of using messageType, and ICU plurals instead of _plural keys.
2 participants
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
