Group event notifications and inbound nukes in quick succession#4530
Open
blontd6 wants to merge 1 commit into
Open
Group event notifications and inbound nukes in quick succession#4530blontd6 wants to merge 1 commit into
blontd6 wants to merge 1 commit into
Conversation
Contributor
|
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}
Contributor
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
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.
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