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 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
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 <[email protected]>
|
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 an alliance-extension flow: radial menu emits a SendAllianceExtensionIntentEvent; EventsDisplay listens, finds the alliance for the recipient, removes renewal events when present, and requests a UI update. Core types, player, and alliance logic expose and enforce extension-related fields and checks. Changes
Sequence DiagramsequenceDiagram
participant Player
participant RadialMenu
participant ActionHandler
participant EventBus
participant EventsDisplay
participant GameCore
Player->>RadialMenu: select "ally_extend"
RadialMenu->>ActionHandler: handleExtendAlliance(recipient)
ActionHandler->>EventBus: emit SendAllianceExtensionIntentEvent(recipient)
EventBus->>EventsDisplay: deliver SendAllianceExtensionIntentEvent
EventsDisplay->>GameCore: lookup current player & alliance by recipient id
GameCore-->>EventsDisplay: alliance or null
alt alliance found
EventsDisplay->>GameCore: removeAllianceRenewalEvents(alliance.id)
EventsDisplay->>RadialMenu: requestUpdate()
RadialMenu->>RadialMenu: refresh gradients and re-render
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 <[email protected]>
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
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