Conversation
Replace clip-path overlay with CSS linear-gradient on the main arc path so the entire button is clickable. Use the 30s extension window (not the full alliance duration) for the timer fraction, and lighten the expired portion to match the name overlay style. Preserve the gradient fill on hover/mouseout so the countdown remains visible during interaction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Preserve SVG animate elements by comparing a canonical state key before re-invoking customRender. Renderers can opt in via customRenderStateKey and handle in-place updates via the update flag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughAdds alliance-extension support: new ally-extend radial menu item with per-item timer gradients, UI emits a SendAllianceExtensionIntentEvent, EventsDisplay handles AllianceExtension updates to remove renewal events and refresh UI, and core exposes AllianceInfo plus agreed-to-extend logic. Changes
Sequence DiagramsequenceDiagram
participant Player
participant RadialMenu
participant ActionHandler
participant EventBus
participant EventsDisplay
participant GameCore
Player->>RadialMenu: choose "ally_extend" on target
RadialMenu->>ActionHandler: call handleExtendAlliance(target)
ActionHandler->>EventBus: emit SendAllianceExtensionIntentEvent(targetId)
EventBus->>GameCore: deliver intent / update
GameCore->>EventBus: emit AllianceExtension update
EventBus->>EventsDisplay: deliver AllianceExtension update
EventsDisplay->>GameCore: remove renewal events for allianceId
EventsDisplay->>RadialMenu: request UI refresh
RadialMenu->>RadialMenu: refresh gradients and re-render items
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/RadialMenuElements.ts (1)
742-755:⚠️ Potential issue | 🔴 Critical
displayedproperty is never evaluated in the rendering pipeline.The
displayedproperty is defined inMenuElementbut never checked during rendering.renderPaths(line 322) only evaluates thedisabledproperty, andisItemDisabled(line 564) ignoresdisplayedentirely. Multiple menu elements definedisplayedcallbacks (lines 184, 200, 215, 231, 332 in RadialMenuElements.ts), but these are never invoked. This means conditional visibility based ondisplayedwill not work. Either remove the unused property or implement evaluation inisItemDisabledand the rendering pipeline.
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 242-254: The local variable name "window" in the timerFraction
function shadows the global window object and should be renamed to a clearer
identifier; inside timerFraction (use params, interaction,
interaction.allianceExpiresAt and
params.game.config().allianceExtensionPromptOffset() to locate), replace const
window = ... with a descriptive name like alliancePromptWindow or promptOffset
and update its usages (remaining / alliancePromptWindow) so the code no longer
hides the global window.
🧹 Nitpick comments (6)
tests/client/graphics/RadialMenuElements.test.ts (1)
352-442: Good coverage of ally_extend visibility and enabled/disabled states.The four tests cover the key scenarios well. One small note: there is repeated mock setup (creating
allyPlayer, overridingmockGame.owner, spreadingmockPlayerActions.interaction) across all four tests. Consider extracting a small helper function to reduce duplication — but this is a minor nit for a test file.♻️ Optional: extract shared setup
function setupAllyExtensionTest( mockParams: MenuElementParams, mockGame: GameView, mockPlayerActions: any, interactionOverrides: Record<string, unknown>, ) { const allyPlayer = { id: () => 2, isAlliedWith: vi.fn(() => true), isPlayer: vi.fn(() => true), } as unknown as PlayerView; mockParams.selected = allyPlayer; mockGame.owner = vi.fn(() => allyPlayer); mockPlayerActions.interaction = { ...mockPlayerActions.interaction, canBreakAlliance: true, ...interactionOverrides, }; }Then each test becomes a one-liner setup + assertions.
src/core/GameRunner.ts (1)
219-228: The extension-window threshold logic is duplicated in multiple places.The same check
alliance.expiresAt() <= ticks + config.allianceExtensionPromptOffset()appears here, inPlayerImpl.canExtendAlliance(lines 427-432), and inEventsDisplay.checkForAllianceExpirations(lines 302-304). If the threshold logic ever changes, all three must be updated together.Consider extracting a shared helper (e.g.,
alliance.isInExtensionWindow(ticks)onAllianceImpl) to keep this in one place.♻️ Suggested: add helper to AllianceImpl
Add to
AllianceImpl:isInExtensionWindow(): boolean { return this.expiresAt_ <= this.mg.ticks() + this.mg.config().allianceExtensionPromptOffset(); }Then replace the duplicated checks in
GameRunner.ts,PlayerImpl.ts, andEventsDisplay.tswithalliance.isInExtensionWindow().src/core/game/PlayerImpl.ts (1)
405-435: RedundantallianceWithcall and dead null-check.
this.allianceWith(other)is called at line 417 (as a boolean guard) and again at line 421 (to get the value). The null check at lines 423-425 is dead code since line 417 already returnsfalsewhen no alliance exists. Simplify by retrieving the alliance once.♻️ Proposed simplification
canExtendAlliance(other: Player): boolean { if (other === this) { return false; } if (this.isDisconnected() || other.isDisconnected()) { return false; } - if (!this.allianceWith(other) || !this.isAlive() || !other.isAlive()) { + + if (!this.isAlive() || !other.isAlive()) { return false; } const alliance = this.allianceWith(other); if (!alliance) { return false; } if ( alliance.expiresAt() > this.mg.ticks() + this.mg.config().allianceExtensionPromptOffset() ) { return false; } return !alliance.agreedToExtend(this); }src/client/graphics/layers/RadialMenu.ts (3)
352-399: Timer gradient setup looks good overall, but thethis.params === nullcheck on line 356 is always false.Inside the
arcs.eachcallback, line 354 already guards with&& this.params, sothis.paramsis guaranteed non-null at line 356. The disabled check can just used.data.disabled(this.params).Suggested fix
arcs.each((d) => { if (d.data.timerFraction && this.params) { const fraction = d.data.timerFraction(this.params); - const disabled = this.params === null || d.data.disabled(this.params); + const disabled = d.data.disabled(this.params); const baseColor = disabled
396-397: Globald3.selectfor path lookup may match elements outside this menu.
d3.select(path[data-id="..."])queries the entire document. If another SVG on the page happens to have a<path>with the samedata-id, this picks the wrong element. This is a pre-existing pattern throughout the file (lines 403, 538, 590), but the new timer-gradient code adds another instance. Consider scoping the selection to the menu's SVG — for example, storing a reference to the menu's<svg>and querying within it.
1131-1163: WhencustomRenderStateKeyis absent, children are cleared before re-render, but when present and state changed, clearing is left tocustomRender(update=true).This split responsibility is a bit fragile — the caller (
refresh()) owns cleanup in one branch but the callee (customRender) owns it in the other. If a futurecustomRenderimplementer forgets to clear children whenupdate=true, stale content will accumulate.Consider making the contract consistent: always clear children here before calling
customRender, or always passupdateand letcustomRenderdecide. Right now the currentallyExtendElement.customRenderhandles it correctly (lines 270–272 in RadialMenuElements.ts), so this is not broken — just worth keeping tidy.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 29-32: The module-level call to getSvgAspectRatio sets
allianceIconAspectRatio at import time and breaks tests because the test mock
doesn't export that function; change this to lazy initialization by creating a
getter/initializer (e.g., getAllianceIconAspectRatio) that calls
getSvgAspectRatio and caches the result, remove the top-level getSvgAspectRatio
invocation, and inside customRender use allianceIconAspectRatio ?? 1 as a
fallback while kicking off getAllianceIconAspectRatio() so subsequent renders
use the real ratio; alternatively, if you must keep eager behavior, update the
test mock to re-export or stub getSvgAspectRatio (or use importOriginal) so the
import-time call doesn't throw.
🧹 Nitpick comments (1)
src/client/graphics/layers/RadialMenuElements.ts (1)
266-330: Consider extracting a helper for the repeated SVG image+animation creation.The left and right handshake icons follow the exact same pattern: create
<image>, set attributes, optionally append<animate>. Pulling that into a small local function removes the duplication and makes intent clearer.Sketch
+ function appendHandshakeIcon( + parent: SVGGElement, + ns: string, + x: number, + y: number, + width: number, + height: number, + disabled: boolean, + animate: boolean, + ) { + const img = document.createElementNS(ns, "image"); + img.setAttribute("href", allianceIcon); + img.setAttribute("width", width.toString()); + img.setAttribute("height", height.toString()); + img.setAttribute("x", x.toString()); + img.setAttribute("y", y.toString()); + img.setAttribute("opacity", disabled ? "0.5" : "1"); + if (animate) { + const anim = document.createElementNS(ns, "animate"); + anim.setAttribute("attributeName", "opacity"); + anim.setAttribute("values", "1;0.2;1"); + anim.setAttribute("dur", "1.5s"); + anim.setAttribute("repeatCount", "indefinite"); + img.appendChild(anim); + } + parent.appendChild(img); + }
Replace eager module-level getSvgAspectRatio call with a lazy getter that fetches and caches on first render, falling back to 1 until the promise resolves. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 295-304: The animation on leftImg (created as animLeft when
myAgreed is false) currently cycles "1;0.2;1" so it overrides the static
leftImg.setAttribute("opacity", disabled ? "0.5" : "1"); change the animate
values to use the disabled ceiling (e.g. if disabled then "0.5;0.1;0.5" else
"1;0.2;1") so the animation never exceeds the disabled opacity, and apply the
same pattern to the right icon's animation (animRight/rightImg) to keep disabled
visuals consistent.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 321-328: The right-side animation block (when otherAgreed is
false) creates animRight and hardcodes values "1;0.2;1", which ignores the
disabled state and overrides the opacity set on rightImg; update the creation of
animRight in RadialMenuElements so that animRight.setAttribute("values", ...)
uses the same conditional as the left side (e.g., disabled ? "0.5;0.1;0.5" :
"1;0.2;1") or otherwise respects the disabled flag before appending animRight to
rightImg; reference the otherAgreed check, the animRight element, and rightImg
when making the change.
…nd-radial-menu # Conflicts: # src/client/graphics/layers/RadialMenuElements.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/graphics/layers/RadialMenu.ts`:
- Around line 1276-1345: The async getSvgAspectRatio call in
renderAllyExtendIcon can resolve after a later render and append duplicate
icons; add a render-token check so only the latest render appends. On entry of
renderAllyExtendIcon (use the content SVGGElement and the update flag), generate
a unique token (e.g. a Symbol or incrementing id) and store it on content (e.g.
content.__allyExtendRenderToken or content.dataset.allyExtendToken); when update
is true refresh the token; capture the token in the then() callback and before
creating/appending leftImg/rightImg verify the stored token still matches the
captured token, returning early if it does not. This uses renderAllyExtendIcon
and getSvgAspectRatio to avoid stale appends.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/game/Game.ts (1)
871-877:canExtendis derivable — risk of implementor inconsistency; consider simplifying the namemyPlayerAgreedToExtend.Two minor design notes:
Redundant field:
canExtendequalsinExtensionWindow && !myPlayerAgreedToExtend. Storing it as a separate field means implementations must keep it in sync. If the only reason it exists is convenience for UI consumers, it is cleaner to drop it from the interface and let callers compute it, or explicitly document what extra logic it encodes if it is not purely derived.Naming:
myPlayerAgreedToExtendembeds a perspective pronoun (my) that is non-idiomatic for TypeScript interfaces.selfAgreedToExtendor simplyagreedToExtend(symmetric withotherAgreedToExtend) is shorter and easier for non-native readers.♻️ Proposed simplification
export interface AllianceInfo { expiresAt: Tick; inExtensionWindow: boolean; - myPlayerAgreedToExtend: boolean; + agreedToExtend: boolean; otherAgreedToExtend: boolean; - canExtend: boolean; }UI consumers can then derive
canExtendlocally asinExtensionWindow && !agreedToExtend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/Game.ts` around lines 871 - 877, The AllianceInfo interface currently stores a derived property and uses a perspective-specific name; remove the redundant canExtend property from AllianceInfo and rename myPlayerAgreedToExtend to a symmetric, perspective-neutral name (e.g., agreedToExtend) so callers can derive canExtend as inExtensionWindow && !agreedToExtend; update all usages of AllianceInfo, AllianceInfo.canExtend, AllianceInfo.myPlayerAgreedToExtend and any consumers (UI code, serializers, tests) to compute canExtend where needed and to reference the new agreedToExtend field (or chosen name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/game/Game.ts`:
- Around line 871-877: The AllianceInfo interface currently stores a derived
property and uses a perspective-specific name; remove the redundant canExtend
property from AllianceInfo and rename myPlayerAgreedToExtend to a symmetric,
perspective-neutral name (e.g., agreedToExtend) so callers can derive canExtend
as inExtensionWindow && !agreedToExtend; update all usages of AllianceInfo,
AllianceInfo.canExtend, AllianceInfo.myPlayerAgreedToExtend and any consumers
(UI code, serializers, tests) to compute canExtend where needed and to reference
the new agreedToExtend field (or chosen name).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/RadialMenuElements.ts (1)
61-63:renderType?: string— consider a typed literal union for compile-time safety.A plain
stringlets any value through silently. Since"allyExtend"is the only value today, a union keeps future additions explicit and typo-proof.♻️ Suggested refactor
- renderType?: string; + renderType?: "allyExtend"; // extend the union as new render types are added🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/RadialMenuElements.ts` around lines 61 - 63, Change the loose string typing for renderType to a literal union so callers get compile-time guarantees: replace renderType?: string on the MenuElement (in RadialMenuElements.ts) with a typed union (e.g., export a RenderType type or inline union that includes 'allyExtend' for now) and update any switch/if checks or assignments that reference renderType to use that type (or add a fallback/default if dynamic values are still possible) so typos and invalid values are caught by the compiler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 231-243: The falsy check `!interaction?.allianceInfo?.expiresAt`
incorrectly treats a Tick value of 0 as missing; update the guard in
`timerFraction` to explicitly check for the presence of `allianceInfo` instead
(e.g., `if (!interaction?.allianceInfo) return 1;`) so you still rely on the
required `expiresAt` number on `MenuElementParams`'s `interaction.allianceInfo`,
then proceed to compute `remaining` using `interaction.allianceInfo.expiresAt`
as before.
---
Nitpick comments:
In `@src/client/graphics/layers/RadialMenuElements.ts`:
- Around line 61-63: Change the loose string typing for renderType to a literal
union so callers get compile-time guarantees: replace renderType?: string on the
MenuElement (in RadialMenuElements.ts) with a typed union (e.g., export a
RenderType type or inline union that includes 'allyExtend' for now) and update
any switch/if checks or assignments that reference renderType to use that type
(or add a fallback/default if dynamic values are still possible) so typos and
invalid values are caught by the compiler.
| timerFraction: (params: MenuElementParams): number => { | ||
| const interaction = params.playerActions?.interaction; | ||
| if (!interaction?.allianceInfo?.expiresAt) return 1; | ||
| const remaining = Math.max( | ||
| 0, | ||
| interaction.allianceInfo.expiresAt - params.game.ticks(), | ||
| ); | ||
| const extensionWindow = Math.max( | ||
| 1, | ||
| params.game.config().allianceExtensionPromptOffset(), | ||
| ); | ||
| return Math.max(0, Math.min(1, remaining / extensionWindow)); | ||
| }, |
There was a problem hiding this comment.
Falsy check on Tick (number) is imprecise — use a null-check instead.
!interaction?.allianceInfo?.expiresAt is true when expiresAt is 0. Since AllianceInfo.expiresAt is a required Tick (non-optional number), the actual guard you need is whether allianceInfo itself is absent. In practice tick-0 alliances are impossible, so the bug is dormant, but the intent is clearer with an explicit null check.
🔧 Suggested fix
- if (!interaction?.allianceInfo?.expiresAt) return 1;
+ if (interaction?.allianceInfo == null) return 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/RadialMenuElements.ts` around lines 231 - 243, The
falsy check `!interaction?.allianceInfo?.expiresAt` incorrectly treats a Tick
value of 0 as missing; update the guard in `timerFraction` to explicitly check
for the presence of `allianceInfo` instead (e.g., `if
(!interaction?.allianceInfo) return 1;`) so you still rely on the required
`expiresAt` number on `MenuElementParams`'s `interaction.allianceInfo`, then
proceed to compute `remaining` using `interaction.allianceInfo.expiresAt` as
before.
2 participants
Description:
The following PR replaces the (disabled) alliance request button with an alliance extension/renewal button when the alliance with the target player is expiring.
Agreeing to renewal via radial menu also hides the message in the EventsDisplay.
Screen.Recording.2026-02-07.at.13.14.59.mov
Icon size adjusted:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
deshack_82603