Add two-stage confirmation for breaking alliances. #2440
Conversation
…ction; fix state reset and D3 pie typing
…rm lit ESM and mock DOM-dependent utils
|
|
WalkthroughThis change implements a two-step confirmation flow for breaking alliances in the radial menu, introduces new state management via RadialMenuState, improves click-handling logic to prevent accidental menu closes, and adds corresponding unit tests. Jest configuration is also updated to exclude additional libraries from transformation. Changes
Sequence DiagramsequenceDiagram
actor User
participant Menu as Radial Menu
participant State as RadialMenuState
participant Handler as handleBreakAlliance
rect rgb(240, 248, 255)
Note over User,Handler: Stage 1: First Click (Set Pending)
User->>Menu: Click ally_break item
Menu->>State: Set breakAlliancePendingId = playerId
State-->>Menu: breakAlliancePendingId updated
Menu->>Menu: Render with confirm_break visuals
Menu-->>User: Menu stays open, item shows confirmation state
end
rect rgb(240, 248, 255)
Note over User,Handler: Stage 2: Second Click (Confirm & Close)
User->>Menu: Click confirm_break item
Menu->>Handler: Call handleBreakAlliance(playerId)
Handler-->>Menu: Alliance broken
Menu->>State: Reset breakAlliancePendingId = null
Menu->>Menu: Close menu
Menu-->>User: Menu closed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 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: 0
🧹 Nitpick comments (4)
src/client/graphics/layers/RadialMenuElements.ts (2)
195-198: Consider encapsulating state instead of using a global mutable object.
RadialMenuStateis a plain object with a mutable property that's shared across modules. This pattern can cause issues if multiple radial menus exist or during testing when state needs isolation.Consider alternatives:
- Pass state through
MenuElementParams- Use a class with methods to manage state
- Create a state management utility with getters/setters
588-609: Consider adding the purple color to the COLORS constant.The confirmation state uses a hardcoded color
"#800080"(line 596). For consistency and maintainability, consider adding this to theCOLORSconstant (e.g.,COLORS.confirmBreak).Apply this diff:
export const COLORS = { build: "#ebe250", building: "#2c2c2c", boat: "#3f6ab1", ally: "#53ac75", breakAlly: "#c74848", + confirmBreak: "#800080", delete: "#ff0000", info: "#64748B",Then update line 596:
ally = { ...allyBreakElement, name: "confirm_break", - color: "#800080", + color: COLORS.confirmBreak, icon: traitorConfirmIcon,tests/radialMenuElements.spec.ts (1)
25-32: Simplify theisAlliedWithstub logic.Lines 28-31 have a ternary that returns
truein both branches, making the condition unnecessary. For the current tests this works since you're testing the ally-break flow where players are allied, but it's misleading.Apply this diff:
const makePlayer = (id: string) => ({ id: () => id, - isAlliedWith: (other: any) => - other && typeof other.id === "function" && other.id() !== id - ? true - : true, + isAlliedWith: () => true, }) as unknown as import("../src/core/game/GameView").PlayerView;src/client/graphics/layers/RadialMenu.ts (1)
478-497: Dynamic refresh enables two-step confirmation UI updates.The menu re-renders after actions to reflect state changes (like pending confirmation). The conditional close logic correctly keeps the menu open only when a break alliance is pending for the currently selected player.
Consider simplifying the condition for clarity:
// Only close if no pending break for this player const isPendingForSelected = RadialMenuState.breakAlliancePendingId === this.params?.selected?.id(); if (!isPendingForSelected) { this.isTransitioning = false; this.hideRadialMenu(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
resources/images/TraitorIconConfirmWhite.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
jest.config.ts(1 hunks)src/client/graphics/layers/RadialMenu.ts(8 hunks)src/client/graphics/layers/RadialMenuElements.ts(4 hunks)tests/radialMenuElements.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/RadialMenuElements.tstests/radialMenuElements.spec.tssrc/client/graphics/layers/RadialMenu.ts
🧬 Code graph analysis (2)
tests/radialMenuElements.spec.ts (2)
src/core/game/AllianceImpl.ts (1)
other(19-24)src/client/graphics/layers/RadialMenuElements.ts (4)
MenuElementParams(29-42)RadialMenuState(196-198)rootMenuElement(579-625)COLORS(72-99)
src/client/graphics/layers/RadialMenu.ts (1)
src/client/graphics/layers/RadialMenuElements.ts (2)
MenuElement(44-59)RadialMenuState(196-198)
🔇 Additional comments (7)
src/client/graphics/layers/RadialMenuElements.ts (1)
200-222: Two-step confirmation logic looks correct.The implementation properly handles:
- First click: sets pending state without closing menu
- Second click: executes break, resets state, closes menu
- Fallback: uses nullish coalescing for undefined selected
tests/radialMenuElements.spec.ts (1)
81-124: Test coverage looks comprehensive.The test suite properly verifies:
- Stage 1: default break state with correct color
- Stage 2: confirm state when pending id matches
- Two-step flow: first click sets pending, second click triggers handler and closes menu
src/client/graphics/layers/RadialMenu.ts (4)
132-138: Click handler guards prevent accidental menu close.The added checks using
event.target.closest(".menu-item-path")correctly prevent the menu from closing when clicking inside menu items, which is essential for the two-step confirmation flow.Also applies to: 159-164
265-270: Cleanup of icon and path state prevents stale references.Properly clearing the
menuIconsandmenuPathsmaps before re-rendering prevents memory leaks and ensures fresh state.
289-294: Stepwise pie generator construction addresses TypeScript inference.Building the d3 pie generator in steps helps TypeScript correctly infer types and avoid call-signature ambiguity. This is a common pattern when working with d3's fluent API in TypeScript.
843-844: State reset on menu close prevents stale confirmation state.Resetting
breakAlliancePendingIdwhen the menu closes ensures the confirmation state doesn't persist across menu reopens, matching the expected behavior.jest.config.ts (1)
18-18: No issues found; transformIgnorePatterns change is correct.Lit packages are legitimate production dependencies used extensively throughout the codebase. The codebase imports from lit, lit/decorators.js, lit/directive.js, lit/directives/*, and lit-markdown across 50+ component files. Including these in transformIgnorePatterns is necessary for Jest to handle ESM modules correctly.
Implement safe-unally feature to prevent accidental alliance breaks: - Requires two clicks: first to initiate, second to confirm - Visual feedback with purple color on confirmation stage - Uses new TraitorIconConfirmWhite icon for confirm state - Maintains state between clicks using RadialMenuState - Menu stays open during confirmation flow - Comprehensive test coverage with 3 new test cases This prevents accidental ally breaks during intense gameplay while maintaining intuitive UX with clear visual indicators.
Hauke12345
changed the title
|
Improved description and updated to main branch. This feature was embraced by Rex :-) |
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
2 participants
Description:
Implements a safe-unalley flow in the radial menu with a two-step confirmation:
First click: toggles a “pending confirm” state, keeps the menu open, changes the slice color to purple, and swaps the icon to TraitorIconConfirmWhite.svg.
Second click: confirms the break, performs the action, and closes the menu.
Closing the menu without confirming resets the pending state (no stuck purple state on reopen).
Additional improvements:
Immediate re-render of the current menu after actions so the break/confirm button updates in-place (no manual reopen needed).
New confirm icon asset: resources/images/TraitorIconConfirmWhite.svg.
D3 pie generator typing fix in RadialMenu.ts (avoids TS2349 call-signature issues).
Unit tests for the break/confirm behavior.
Jest config updated to transform lit ESM packages and avoid DOM coupling in tests.
Implementation details
State
Introduced RadialMenuState.breakAlliancePendingId for safe mutation and cross-module use.
Menu building (RadialMenuElements.ts)
Base allyBreakElement is static; dynamic behavior is applied in rootMenuElement.subMenu:
Stage 1 (normal): name "break", color COLORS.breakAlly, icon TraitorIconWhite.svg.
Stage 2 (confirm): name "confirm_break", color "#800080", icon TraitorIconConfirmWhite.svg.
Action toggles pending state on first click; on second click triggers handleBreakAlliance, closes the menu, resets state.
Rendering/behavior (RadialMenu.ts)
After a leaf action, recompute root menu items and re-render to reflect state changes immediately.
Clear stale entries in menuIcons/menuPaths during re-render to avoid leftover visuals.
hideRadialMenu resets RadialMenuState.breakAlliancePendingId to null.
D3 pie generator configured stepwise to avoid TS callable inference issues.
Tests
Added tests/radialMenuElements.spec.ts:
Stage 1 shows break with default color.
Stage 2 shows confirm with purple when pending id matches.
First click sets pending without closing; second click confirms, calls handler, resets state, and closes menu.
Tests mock BuildMenu (to avoid lit import) and Utils.translateText/getSvgAspectRatio (to avoid document access).
Jest
jest.config.ts transformIgnorePatterns updated to transform ESM packages (lit, lit-html, lit-element, @lit, @LIT-labs) used indirectly by client code.
Break (stage 1): slice in break color, icon = TraitorIconWhite.svg
Confirm (stage 2): purple slice, icon = TraitorIconConfirmWhite.svg
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME
hauke4707