feat: pin alliance requests/renewals above scrollable event feed#3178
feat: pin alliance requests/renewals above scrollable event feed#3178Skigim wants to merge 3 commits intoopenfrontio:mainfrom
Conversation
Addresses openfrontio#3172 - Alliance requests and renewal prompts now render in a dedicated non-scrolling section between the button bar and the scrollable events feed. This prevents them from being lost as other messages arrive. - Split filteredEvents into pinnedEvents (ALLIANCE_REQUEST, RENEW_ALLIANCE with action buttons) and regularEvents - Pinned section has a yellow-tinted style with its own max-height/scroll for edge cases with many simultaneous requests - Auto-collapses when no pending alliance events exist - All existing button actions (accept/reject/renew/ignore/focus) work identically
|
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 a new AllianceDisplay UI layer, refactors event handling by extracting event partitioning/sorting APIs, reworks event/attack/boat rendering and actions, updates renderer wiring and index HTML, and adds tests for event splitting/sorting. Changes
Sequence DiagramsequenceDiagram
participant Game as Game State
participant Renderer as GameRenderer
participant Alliance as AllianceDisplay
participant Events as EventsDisplay / Processor
participant UI as DOM Renderer
rect rgba(200,200,255,0.5)
Game->>Renderer: provide game, eventBus, uiState
Renderer->>Alliance: inject game, eventBus, uiState
Renderer->>Events: inject game, eventBus, uiState
end
rect rgba(200,255,200,0.5)
Game->>Alliance: alliance updates / ticks
Alliance->>Alliance: queue/pin alliance events, manage expirations
Alliance->>UI: render pinned + informational alliance events
end
rect rgba(255,220,200,0.5)
Game->>Events: provide global events & attack/boat state
Events->>Events: splitAndSortEvents(events)
Events->>UI: render pinned events then regular events with actions
UI->>Game: emit navigation / cancel intents via eventBus
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/EventsDisplay.ts`:
- Around line 1035-1066: EventsDisplay references undeclared reactive properties
(outgoingAttacks, outgoingLandAttacks, outgoingBoats, incomingAttacks) used by
the template and methods like renderOutgoingAttacks, renderOutgoingLandAttacks,
and renderBoats; add these properties to the EventsDisplay class (e.g., declare
them with `@state`() or proper class fields) or otherwise obtain them from the
correct source (e.g., accept them as `@property` inputs or call the shared data
provider used by AttacksDisplay) so the template bindings resolve at runtime;
ensure the property names and types match those used in AttacksDisplay and
update any initialization/updates where EventsDisplay currently expects that
data.
🧹 Nitpick comments (3)
src/client/graphics/layers/EventsDisplay.ts (2)
66-101:splitAndSortEventsis clear and testable — nice extraction.Two small things worth noting:
- The filter predicate is written twice (once normal, once negated). A single-pass split avoids the duplication and is a bit faster for large lists.
- The default priority
100000is a magic number. A named constant would make the sort intent clearer.Single-pass split + named constant
+const DEFAULT_PRIORITY = 100_000; + +function isPinnedEvent(event: GameEvent): boolean { + return ( + (event.type === MessageType.ALLIANCE_REQUEST || + event.type === MessageType.RENEW_ALLIANCE) && + !!event.buttons && + event.buttons.length > 0 + ); +} + export function splitAndSortEvents(events: GameEvent[]): { pinnedEvents: GameEvent[]; regularEvents: GameEvent[]; } { - const pinnedEvents = events.filter( - (event) => - (event.type === MessageType.ALLIANCE_REQUEST || - event.type === MessageType.RENEW_ALLIANCE) && - event.buttons && - event.buttons.length > 0, - ); - const regularEvents = events.filter( - (event) => - !( - (event.type === MessageType.ALLIANCE_REQUEST || - event.type === MessageType.RENEW_ALLIANCE) && - event.buttons && - event.buttons.length > 0 - ), - ); + const pinnedEvents: GameEvent[] = []; + const regularEvents: GameEvent[] = []; + + for (const event of events) { + (isPinnedEvent(event) ? pinnedEvents : regularEvents).push(event); + } regularEvents.sort((a, b) => { - const aPrior = a.priority ?? 100000; - const bPrior = b.priority ?? 100000; + const aPrior = a.priority ?? DEFAULT_PRIORITY; + const bPrior = b.priority ?? DEFAULT_PRIORITY; if (aPrior === bPrior) { return a.createdAt - b.createdAt; } return bPrior - aPrior; }); return { pinnedEvents, regularEvents }; }
874-935: Duplicate button-rendering logic between pinned and regular sections.The click handler + button template (lines 897–929 for pinned, 976–1012 for regular) is nearly identical. If you ever change the button style or behavior, you need to update both places. Extract a shared helper like
renderEventButtons(event)to keep things DRY.Extract a shared button renderer
Add a private method to
EventsDisplay:private renderEventButtons(event: GameEvent) { if (!event.buttons) return ""; return html` <div class="flex flex-wrap gap-1.5 mt-1"> ${event.buttons.map( (btn) => html` <button class="inline-block px-3 py-1 text-white rounded-sm text-md md:text-sm cursor-pointer transition-colors duration-300 ${btn.className.includes("btn-info") ? "bg-blue-500 hover:bg-blue-600" : btn.className.includes("btn-gray") ? "bg-gray-500 hover:bg-gray-600" : "bg-green-600 hover:bg-green-700"}" `@click`=${() => { btn.action(); if (!btn.preventClose) { const idx = this.events.findIndex((e) => e === event); if (idx !== -1) this.removeEvent(idx); } this.requestUpdate(); }} > ${btn.text} </button> `, )} </div> `; }Then use
${this.renderEventButtons(event)}in both the pinned and regular sections.Also applies to: 976-1012
tests/client/graphics/layers/EventsDisplay.test.ts (1)
13-20: Small note: theas GameEventcast hides missing optional fields.This works fine today since the spread covers all required fields. But if
GameEventgains a new required field in the future, this cast will silently suppress the type error. Consider usingOmitorPickpatterns to keep the compiler helping you, or just leave it — it is a test helper after all.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #3172 by pinning alliance requests and renewal prompts in a dedicated non-scrolling section above the scrollable events feed, preventing them from being lost as other messages arrive.
Changes:
- Extracted a new
splitAndSortEventshelper function to separate actionable alliance events (ALLIANCE_REQUEST, RENEW_ALLIANCE with buttons) from regular events - Added a new pinned section with yellow-tinted styling between the button bar and scrollable feed
- Added comprehensive test coverage for the
splitAndSortEventsfunction
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/client/graphics/layers/EventsDisplay.test.ts | New test file with comprehensive coverage for splitAndSortEvents function, including edge cases for pinning logic and sorting behavior |
| src/client/graphics/layers/EventsDisplay.ts | Exported GameEvent interface, added splitAndSortEvents function, and implemented pinned events UI section with button handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Skigim
force-pushed
the
pr/pinned-alliance-events
branch
from
77158fe to
38d90fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/EventsDisplay.ts`:
- Around line 922-934: Delete the unused private method handleRetaliate from the
EventsDisplay class: remove the entire function definition (private
handleRetaliate(attack: AttackUpdate) { ... }) since it is never referenced;
ensure no remaining references to attacker, myPlayer, counterTroops, or
SendAttackIntentEvent are left behind in EventsDisplay and run tests/tsc to
confirm no imports or usages are required elsewhere.
- Around line 1272-1277: The empty-state template in EventsDisplay incorrectly
uses this.incomingAttacks to determine whether to render the "no events" row
even though incoming attacks are rendered elsewhere; update the empty-state
guard by removing the incomingAttacks length check so the placeholder only
depends on events and outgoing attack lists (i.e., remove the
`this.incomingAttacks.length === 0` condition from the template), and ensure the
property incomingAttacks (populated at line 322) is not relied on elsewhere in
this component for rendering.
- Line 126: The field incomingBoatIDs on EventsDisplay is populated in
updateAttacksAndBoats but never consumed, causing unbounded growth; either
remove the incomingBoatIDs declaration and all writes to it in
updateAttacksAndBoats (and any code that assumes it exists), or implement
consumption logic where EventsDisplay needs to act on new boat events (e.g.,
process and clear IDs after handling) — locate the incomingBoatIDs Set<number>
and the updateAttacksAndBoats method in EventsDisplay and either delete the
field and its pushes or add a clear/processing step that reads the IDs and
empties the set once handled.
🧹 Nitpick comments (3)
src/client/graphics/layers/EventsDisplay.ts (3)
82-99: Duplicated filter condition between pinned and regular partitions.The pinning predicate is written twice (lines 83–88 and 93–97). If the criteria change, both must stay in sync. A single-pass partition avoids this:
♻️ Single-pass partition
- const pinnedEvents = events - .filter( - (event) => - (event.type === MessageType.ALLIANCE_REQUEST || - event.type === MessageType.RENEW_ALLIANCE) && - event.buttons && - event.buttons.length > 0, - ) - .sort((a, b) => a.createdAt - b.createdAt); - const regularEvents = events.filter( - (event) => - !( - (event.type === MessageType.ALLIANCE_REQUEST || - event.type === MessageType.RENEW_ALLIANCE) && - event.buttons && - event.buttons.length > 0 - ), - ); + const pinnedEvents: GameEvent[] = []; + const regularEvents: GameEvent[] = []; + + for (const event of events) { + const isPinned = + (event.type === MessageType.ALLIANCE_REQUEST || + event.type === MessageType.RENEW_ALLIANCE) && + event.buttons && + event.buttons.length > 0; + (isPinned ? pinnedEvents : regularEvents).push(event); + } + + pinnedEvents.sort((a, b) => a.createdAt - b.createdAt);
871-1068: ~200 lines of attack/boat rendering logic duplicated fromAttacksDisplay.
renderOutgoingAttacks,renderOutgoingLandAttacks,renderBoats,getBoatSpriteDataURL,attackWarningOnClick, sprite caching, cancel-intent emitters — all appear to be copy-pasted fromAttacksDisplay. This creates a maintenance burden: a bug fix or style change in one place must be mirrored in the other.Consider extracting shared helpers (plain functions or a shared mixin) that both components can reuse, rather than maintaining two copies.
296-296: UnconditionalrequestUpdate()on every tick.Line 296 forces a Lit re-render every tick regardless of whether any state changed. The
@state()decorators on attack/boat arrays already trigger updates when reassigned, and lines 291–294 handle event-list changes. This unconditional call adds unnecessary rendering overhead.♻️ Remove the unconditional call
- this.requestUpdate(); }If there is a specific edge case this covers, consider guarding it explicitly.
Skigim
force-pushed
the
pr/pinned-alliance-events
branch
2 times, most recently
from
094189b to
133d328
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/EventsDisplay.ts`:
- Around line 904-924: The initial guard checks for !== undefined but
playerBySmallID() never returns undefined; replace that outer undefined check
with a runtime type guard using playerView.isPlayer(). Concretely, call const
playerView = this.game.playerBySmallID(attack.attackerID); then if
(playerView.isPlayer()) { const pv = playerView as PlayerView; await
pv.attackAveragePosition(...); /* keep the existing instanceof PlayerView logic
if still needed or replace with the cast above */ ... } else {
this.emitGoToPlayerEvent(attack.attackerID); } ensuring you reference
playerBySmallID, isPlayer, attackAveragePosition, emitGoToPlayerEvent and
preserve the existing GoToPositionEvent emission path.
In `@tests/client/graphics/layers/EventsDisplay.test.ts`:
- Around line 1-6: Prettier formatting is failing for the test file that imports
describe/expect/it and references MessageType, GameEvent, and
splitAndSortEvents; run the formatter (npx prettier --write <the test file>) or
apply consistent Prettier rules to normalize import spacing and line breaks so
the imports and the top of the test file conform to project style, then re-run
CI to ensure formatting errors are resolved.
🧹 Nitpick comments (2)
src/client/graphics/layers/EventsDisplay.ts (2)
82-115: Extract the pinned-event predicate to avoid repeating the same condition.The filter condition for pinned events appears twice — once positive (line 88-92) and once negated (line 97-101). If the pinning rule changes, both spots must stay in sync. Pull it into a small helper function.
♻️ Proposed refactor
+function isPinnedEvent(event: GameEvent): boolean { + return ( + (event.type === MessageType.ALLIANCE_REQUEST || + event.type === MessageType.RENEW_ALLIANCE) && + !!event.buttons && + event.buttons.length > 0 + ); +} + export function splitAndSortEvents(events: GameEvent[]): { pinnedEvents: GameEvent[]; regularEvents: GameEvent[]; } { const pinnedEvents = events - .filter( - (event) => - (event.type === MessageType.ALLIANCE_REQUEST || - event.type === MessageType.RENEW_ALLIANCE) && - event.buttons && - event.buttons.length > 0, - ) + .filter(isPinnedEvent) .sort((a, b) => a.createdAt - b.createdAt); - const regularEvents = events.filter( - (event) => - !( - (event.type === MessageType.ALLIANCE_REQUEST || - event.type === MessageType.RENEW_ALLIANCE) && - event.buttons && - event.buttons.length > 0 - ), - ); + const regularEvents = events.filter((event) => !isPinnedEvent(event));
310-342:updateAttacksAndBoatsduplicates logic already inAttacksDisplay— consider sharing.This method mirrors the attack/boat state tracking from
AttacksDisplay. Having two independent copies that compute the same derived state from the same game data is fragile — if the filtering logic changes in one place, the other can drift. Consider extracting a shared helper or having one component provide this data to the other.
Skigim
force-pushed
the
pr/pinned-alliance-events
branch
from
133d328 to
0afa94e
Compare
There was a problem hiding this comment.
Can we move all the alliance related ux to a new Layer, outside of EventsDispaly
There was a problem hiding this comment.
can do - want me to keep the pinned events split in place? I think could still be useful for other items that should be sticky eventually
There was a problem hiding this comment.
sure, maybe in another PR?
|
@evanpelle #2906 this looks a lot more pleasing IMO |
|
@ryanbarlow97 I think it's more about the code than the look. I can do something similar or possibly use toasts. If I'm not mistaken Evan just wants the methods for alliance related UI to be separate from the general event feed. I'll mess with it this evening |
sure, try and emulate the paradox feeling, we have quite a big crossover |
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Looks like the text is black on black
There was a problem hiding this comment.
sure, maybe in another PR?
|
@evanpelle pulling alliance requests/renew and alliance debuff timer into new allianceDisplay.ts and adding shared typing. Will keep the look as is (will fix the black on black tho). May open a separate PR to standardize event categorization further - maybe economyDisplay, combatDisplay, etc. |
Skigim
force-pushed
the
pr/pinned-alliance-events
branch
from
0afa94e to
72b89cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/AllianceDisplay.ts`:
- Around line 377-381: The guard checking for falsy player is dead code because
playerBySmallID(...) returns Player | TerraNullius, so replace the current
undefined/null check by verifying the returned object's type with .isPlayer();
specifically, in emitGoToPlayerEvent use const target =
this.game.playerBySmallID(playerID) and only call this.eventBus.emit(new
GoToPlayerEvent(target)) when target.isPlayer() is true (otherwise return),
ensuring you reference playerBySmallID and emitGoToPlayerEvent to locate the
change.
- Around line 64-76: Change the mutable declaration of remainingEvents to an
immutable one: in AllianceDisplay (file contains the events filtering block
using this.events.filter) replace "let remainingEvents" with "const
remainingEvents" since remainingEvents is never reassigned; keep the rest of the
filter logic and the subsequent length comparison/update of this.events
unchanged.
- Around line 336-338: Remove the redundant nullish guard and use the explicit
TerraNullius type check: after obtaining other via
this.game.playerBySmallID(otherID) (cast/typed as PlayerView), replace the `if
(!other || !myPlayer.isAlive() || !other.isAlive()) return;` with a check that
calls `other.isPlayer()` (e.g. `if (!other.isPlayer() || !myPlayer.isAlive() ||
!other.isAlive()) return;`) so you rely on the declared return types and avoid
dead `!other` checks or optional chaining.
🧹 Nitpick comments (6)
src/client/graphics/layers/EventsDisplay.ts (3)
69-106: Extract the "is pinned alliance event" check into a small helper to avoid repeating the same predicate.The filter condition on lines 78–83 and 88–93 is identical (just negated). A simple
isPinnedAllianceEvent(e)function would make both filters a one-liner and keep the logic in one place.♻️ Suggested diff
+function isPinnedAllianceEvent(event: GameEvent): boolean { + return ( + (event.type === MessageType.ALLIANCE_REQUEST || + event.type === MessageType.RENEW_ALLIANCE) && + !!event.buttons && + event.buttons.length > 0 + ); +} + export function splitAndSortEvents(events: GameEvent[]): { pinnedEvents: GameEvent[]; regularEvents: GameEvent[]; } { - const pinnedEvents = events - .filter( - (event) => - (event.type === MessageType.ALLIANCE_REQUEST || - event.type === MessageType.RENEW_ALLIANCE) && - event.buttons && - event.buttons.length > 0, - ) + const pinnedEvents = events + .filter(isPinnedAllianceEvent) .sort((a, b) => a.createdAt - b.createdAt); - const regularEvents = events.filter( - (event) => - !( - (event.type === MessageType.ALLIANCE_REQUEST || - event.type === MessageType.RENEW_ALLIANCE) && - event.buttons && - event.buttons.length > 0 - ), - ); + const regularEvents = events.filter((e) => !isPinnedAllianceEvent(e));This same predicate is also copy-pasted in
AllianceDisplay.render()(lines 498–516 of AllianceDisplay.ts). With a shared helper, all three spots collapse to a single call.
316-366:renderEventButtons,renderEventContent,getEventDescription, andrenderButtonare duplicated verbatim in AllianceDisplay.ts.Both components carry identical copies of these four methods. Consider pulling them into a small shared module (plain functions that accept
events+ dependencies) so changes only need to happen once.
287-299:myPlayer.outgoingAttacks()is called twice per tick.Lines 288–290 and 292–294 each call
myPlayer.outgoingAttacks(), scanning the same list but with opposite filters. Call it once and split the result to cut redundant work on every tick.♻️ Suggested diff
private updateAttacksAndBoats(myPlayer: PlayerView) { - this.outgoingAttacks = myPlayer - .outgoingAttacks() - .filter((a) => a.targetID !== 0); - - this.outgoingLandAttacks = myPlayer - .outgoingAttacks() - .filter((a) => a.targetID === 0); + const allAttacks = myPlayer.outgoingAttacks(); + this.outgoingAttacks = allAttacks.filter((a) => a.targetID !== 0); + this.outgoingLandAttacks = allAttacks.filter((a) => a.targetID === 0); this.outgoingBoats = myPlayer .units() .filter((u) => u.type() === UnitType.TransportShip); }src/client/graphics/layers/AllianceDisplay.ts (3)
45-79: No upper bound onthis.eventslength.
EventsDisplay.tick()caps events at 30 (lines 268–270 in EventsDisplay.ts).AllianceDisplayhas no such cap. In a long-lived game with many alliance events, the list grows without limit. Consider adding a similar cap or confirm that alliance events are naturally scarce enough to skip it.
365-373:removeAllianceRenewalEventsispublicbut only called internally (line 271) and in tests.Mark it
privatefor a cleaner API surface. Tests already bypass visibility withas any.♻️ Fix
- removeAllianceRenewalEvents(allianceID: number) { + private removeAllianceRenewalEvents(allianceID: number) {
492-516: Pinning logic is duplicated fromsplitAndSortEvents— consider reusing the exported helper.
AllianceDisplay.render()manually re-implements the same filter thatsplitAndSortEvents(exported fromEventsDisplay.ts) already provides. Using the shared function would keep the pinning criteria in one place.♻️ Suggested change
+import { splitAndSortEvents } from "./EventsDisplay"; // ... render() { // ... - const pinnedEvents = this.events - .filter( - (e) => - (e.type === MessageType.ALLIANCE_REQUEST || - e.type === MessageType.RENEW_ALLIANCE) && - e.buttons && - e.buttons.length > 0, - ) - .sort((a, b) => a.createdAt - b.createdAt); - - const infoEvents = this.events.filter( - (e) => - !( - (e.type === MessageType.ALLIANCE_REQUEST || - e.type === MessageType.RENEW_ALLIANCE) && - e.buttons && - e.buttons.length > 0 - ), - ); + const { pinnedEvents, regularEvents: infoEvents } = + splitAndSortEvents(this.events);Note:
GameEventis already imported fromEventsDisplay, so addingsplitAndSortEventsto that import is straightforward.
…rmance; enhance DOM handling in EventsDisplay
|
Should be clean now - all Alliance UX is in the dedicated AllianceDisplay - EventDisplay holds a slot and AllianceDisplay is repositioned at first render (hidden initially) |
3 participants
Addresses #3172
Description:
Alliance requests and renewal prompts now render in a dedicated non-scrolling section between the button bar and the scrollable events feed. This prevents them from being lost as other messages arrive.
Please complete the following:
EventsDisplay.render()- extracted to sharedsplitAndSortEvents()helper for cleanliness and testability.Added tests in
tests/client/graphics/layers/EventsDisplay.test.tsPlease put your Discord username so you can be contacted if a bug or regression is found:
skigim