Skip to content

Conversation

@ryanbarlow97
Copy link
Contributor

@ryanbarlow97 ryanbarlow97 commented Dec 30, 2025

closes #2678

Description:

Update to enable bomb flip support by mobile users too

image

Also, I slightly updated the player panel to make it more even and take up less space;

  • removed the huge header bar which took up too much space
  • fixed ui divider spacing

Before:
image
After:
image

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

w.o.n

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 30, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a rocket-direction toggle to the PlayerPanel, extends SwapRocketDirectionEvent to carry rocketDirectionUp: boolean, updates input emission to pass the new value, updates trajectory preview to consume the event, and adds three translation keys for the new UI control. (≤50 words)

Changes

Cohort / File(s) Summary
Translation Keys
resources/lang/en.json
Adds player_panel.arc_up, player_panel.arc_down, and player_panel.flip_rocket_trajectory to support the new toggle UI.
Input / Event Emission
src/client/InputHandler.ts
SwapRocketDirectionEvent now carries rocketDirectionUp: boolean; input logic computes next direction and emits the event with the boolean payload.
Trajectory Preview
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts
Event handler updated to accept the event object and set UI state from event.rocketDirectionUp, then clears lastTargetTile to force trajectory recompute.
Player Panel UI
src/client/graphics/layers/PlayerPanel.ts
Subscribes to SwapRocketDirectionEvent; adds local state sync, handleToggleRocketDirection, and renderRocketDirectionToggle; integrates the toggle into the self-view and adjusts panel layout/spacing.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant PlayerPanel
    participant InputHandler
    participant EventBus as EventSystem
    participant NukeLayer as NukeTrajectoryPreviewLayer

    User->>PlayerPanel: Click rocket direction toggle
    PlayerPanel->>PlayerPanel: handleToggleRocketDirection()
    PlayerPanel->>InputHandler: emit SwapRocketDirectionEvent(nextDirection)

    rect rgb(220, 235, 255)
      Note over InputHandler,EventBus: publish event to central bus
      InputHandler->>EventBus: publish SwapRocketDirectionEvent(rocketDirectionUp)
    end

    EventBus->>PlayerPanel: notify SwapRocketDirectionEvent
    PlayerPanel->>PlayerPanel: set rocketDirectionUp (from event) and re-render

    EventBus->>NukeLayer: notify SwapRocketDirectionEvent
    rect rgb(245, 235, 255)
      Note over NukeLayer: update state & recompute trajectory
      NukeLayer->>NukeLayer: set rocketDirectionUp (from event)
      NukeLayer->>NukeLayer: clear lastTargetTile -> recompute
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Bomb Direction #2435: Modifies SwapRocketDirectionEvent, input emission, and trajectory preview handling — strong overlap with this change.
  • Improve player panel #2060: Touches PlayerPanel.ts and localization keys; related UI and translation edits.

Suggested labels

Translation

Suggested reviewers

  • LoackyBit
  • HimangshuPronoy
  • evanpelle

Poem

🚀 A tap, a flip, the arc turns round,
Tiny button, mighty sound,
Preview clears and paths redraw,
Players smile at one small flaw—gone.

Pre-merge checks

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title 'add mobile bomb flip support' is concise and directly describes the main feature addition: enabling bomb flip functionality for mobile users.
Description check ✅ Passed Description is well-related to the changeset, explaining the mobile bomb flip feature, UI improvements, and linking to issue #2678; it includes testing checklist and screenshots.
Linked Issues check ✅ Passed The PR implements bomb flip support for mobile users via a toggle in the player panel [#2678], though the design uses a dedicated UI element rather than settings menu as originally discussed.
Out of Scope Changes check ✅ Passed Changes include translation keys for bomb flip, input handling refactoring, trajectory preview updates, and player panel UI improvements (header removal, spacing fixes) all aligned with the mobile bomb flip feature and UI refinement objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf6f0b1 and a51a358.

📒 Files selected for processing (1)
  • resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • resources/lang/en.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Deploy to openfront.dev

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

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/PlayerPanel.ts (1)

1-961: Fix Prettier formatting issues.

The pipeline reports code style issues. Run npx prettier --write on this file before merging.

#!/bin/bash
# Check which lines have formatting issues
npx prettier --check "src/client/graphics/layers/PlayerPanel.ts" 2>&1 || true
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b569e68 and 40ef398.

📒 Files selected for processing (4)
  • resources/lang/en.json
  • src/client/InputHandler.ts
  • src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts
  • src/client/graphics/layers/PlayerPanel.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.

Applied to files:

  • src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.

Applied to files:

  • src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.

Applied to files:

  • resources/lang/en.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.

Applied to files:

  • resources/lang/en.json
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.

Applied to files:

  • src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.

Applied to files:

  • src/client/graphics/layers/PlayerPanel.ts
🧬 Code graph analysis (3)
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts (1)
src/client/InputHandler.ts (1)
  • SwapRocketDirectionEvent (92-94)
src/client/InputHandler.ts (1)
src/core/EventBus.ts (1)
  • GameEvent (1-1)
src/client/graphics/layers/PlayerPanel.ts (1)
src/client/InputHandler.ts (1)
  • SwapRocketDirectionEvent (92-94)
🪛 GitHub Actions: 🧪 CI
src/client/graphics/layers/PlayerPanel.ts

[warning] 1-1: Code style issues found. Run 'npx prettier --write' to fix.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Deploy to openfront.dev
🔇 Additional comments (12)
resources/lang/en.json (1)

699-702: LGTM! Translation keys added correctly.

New keys follow the existing naming convention and are properly placed under player_panel. Based on learnings, only en.json should be updated in regular PRs, which is correctly followed here.

src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts (1)

59-63: Clean event-driven state update.

Handler correctly reads the explicit rocketDirectionUp boolean from the event instead of toggling internally. This keeps the state synchronized across all listeners of SwapRocketDirectionEvent.

src/client/InputHandler.ts (2)

92-94: Good: Event now carries explicit direction state.

The SwapRocketDirectionEvent now includes a rocketDirectionUp boolean, making the event self-describing. Consumers no longer need to track or toggle state themselves—they just read the value from the event.


437-439: LGTM! State toggle and event emission are correct.

The handler computes the next direction, updates the local uiState, and emits the event with the new value. This keeps the emitter as the source of truth for the toggle action.

src/client/graphics/layers/PlayerPanel.ts (8)

17-21: LGTM! Import added correctly.

The SwapRocketDirectionEvent import is properly added alongside existing events from InputHandler.


72-72: State property added for rocket direction.

The @state() decorator ensures re-renders when rocketDirectionUp changes. Default true is overridden in init() from shared uiState.


85-89: Event subscription keeps local state in sync.

When another component (like InputHandler via keyboard shortcut) emits SwapRocketDirectionEvent, this listener updates both the shared uiState and local state, then triggers a re-render.


104-106: Safe initialization from shared state.

Uses nullish coalescing to fall back to the existing default if uiState is not yet available.


313-320: Toggle handler emits event for cross-component sync.

The handler updates both local and shared state, then emits SwapRocketDirectionEvent so other listeners (like NukeTrajectoryPreviewLayer) can react. The requestUpdate() call is redundant since state is already updated via @state() decorator, but harmless.


537-555: Rocket direction toggle UI is clear and accessible.

The button shows the current arc direction and uses translateText() for localization. The emoji icon provides a visual cue.

One small note: the translate="no" attribute on the arc label span is good—it prevents translation services from altering the dynamic text.


891-898: Close button repositioned to header.

The absolutely positioned close button is now in the panel header. Clean placement with good hover state (hover:bg-red-500).


932-934: Rocket toggle shown only in self-view.

The toggle is conditionally rendered when viewing your own panel (other === my), which makes sense—players should only control their own rocket trajectory direction.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 30, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/client/graphics/layers/PlayerPanel.ts (3)

72-72: Consider eliminating dual state storage.

The rocketDirectionUp is stored in both local @state() and uiState.rocketDirectionUp. Since you always sync them together (lines 86-87, 317-318), the local copy may be redundant. You could read directly from uiState in your render method.

That said, keeping a local reactive copy might help Lit detect changes more reliably, so this pattern may be intentional for performance.


314-321: Consider adding a defensive null check.

The method logic is correct, but it directly accesses this.uiState.rocketDirectionUp without checking if uiState exists. While uiState is declared as a required property, adding a guard would make the code more defensive:

if (!this.uiState) return;

However, if uiState is guaranteed to be initialized before this component is used, the current code is acceptable.


744-792: Action layout restructuring looks correct.

The conditional logic properly separates actions based on whether you're viewing your own panel vs. another player's:

  • For other players: embargo and alliance buttons
  • For self: embargo all buttons (rendered later at line 793)

The nested ternary for embargo/stop-embargo (lines 750-767) is a bit deep but follows the existing pattern and is still readable.

Optional refinement: The other === my check appears multiple times. You could extract it:

const isSelf = other === my;

But this is purely a readability suggestion.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40ef398 and 460d0f3.

📒 Files selected for processing (1)
  • src/client/graphics/layers/PlayerPanel.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.

Applied to files:

  • src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.

Applied to files:

  • src/client/graphics/layers/PlayerPanel.ts
🔇 Additional comments (6)
src/client/graphics/layers/PlayerPanel.ts (6)

17-21: LGTM: Import structure is clean.

The SwapRocketDirectionEvent import is properly grouped with related UI events. Follows the existing code style.


85-89: LGTM: Event synchronization is correct.

The handler properly syncs the rocket direction state from the event payload and triggers a re-render. Aligns with the event structure used in InputHandler.ts.


105-107: LGTM: Initialization from shared state is correct.

Properly restores rocket direction from uiState on init, with a safe fallback using nullish coalescing.


538-558: LGTM: Clean UI implementation with proper i18n.

The rocket direction toggle UI is well-structured:

  • Proper use of translateText() for all user-facing strings
  • Good UX with clear current state display (arc up/down)
  • The translate="no" attribute on the dynamic state text is a good practice
  • Accessible button with proper event handling

895-902: LGTM: Close button repositioning is correct.

The absolutely positioned close button is properly implemented with:

  • Appropriate z-index for overlay UI
  • Accessible labels using translateText()
  • Clean hover/transition styling
  • Proper event handling

937-938: LGTM: Feature integration is well-placed.

The rocket direction toggle is correctly restricted to the self-view (other === my). Placing it after the Resources section and before Stats makes logical sense for the UI flow.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 30, 2025
@ryanbarlow97 ryanbarlow97 changed the title add mobile flip support [DRAFT] add bomb mobile flip support [DRAFT] Dec 30, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/PlayerPanel.ts (1)

507-507: Fix CSS class spacing.

Missing space between w-[5ch] and font-semibold will cause the classes to merge incorrectly.

🔎 Proposed fix
-          <span translate="no" class="tabular-nums w-[5ch]font-semibold">
+          <span translate="no" class="tabular-nums w-[5ch] font-semibold">
🧹 Nitpick comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)

737-785: Consider extracting nested action button logic for clarity.

The restructured action rendering works correctly, but the nested ternary operators and the redundant other !== my check (line 741) inside the else branch make the code harder to scan.

💡 Optional refactor for improved readability

Extract the other-player actions into a helper method:

private renderOtherPlayerActions(my: PlayerView, other: PlayerView) {
  const canEmbargo = this.actions?.interaction?.canEmbargo;
  const canBreakAlliance = this.actions?.interaction?.canBreakAlliance;
  const canSendAllianceRequest = this.actions?.interaction?.canSendAllianceRequest;

  const embargoButton = canEmbargo
    ? actionButton({
        onClick: (e: MouseEvent) => this.handleEmbargoClick(e, my, other),
        icon: stopTradingIcon,
        iconAlt: "Stop Trading",
        title: translateText("player_panel.stop_trade"),
        label: translateText("player_panel.stop_trade"),
        type: "yellow",
      })
    : actionButton({
        onClick: (e: MouseEvent) => this.handleStopEmbargoClick(e, my, other),
        icon: startTradingIcon,
        iconAlt: "Start Trading",
        title: translateText("player_panel.start_trade"),
        label: translateText("player_panel.start_trade"),
        type: "green",
      });

  return html`
    <div class="grid auto-cols-fr grid-flow-col gap-1">
      ${embargoButton}
      ${canBreakAlliance ? actionButton({...}) : ""}
      ${canSendAllianceRequest ? actionButton({...}) : ""}
    </div>
  `;
}

Then use: ${other === my ? html`` : this.renderOtherPlayerActions(my, other)}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 460d0f3 and fb9e189.

📒 Files selected for processing (1)
  • src/client/graphics/layers/PlayerPanel.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.

Applied to files:

  • src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.

Applied to files:

  • src/client/graphics/layers/PlayerPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (1)
src/client/InputHandler.ts (1)
  • SwapRocketDirectionEvent (92-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Deploy to openfront.dev
🔇 Additional comments (4)
src/client/graphics/layers/PlayerPanel.ts (4)

17-21: LGTM: Import is correct.

The SwapRocketDirectionEvent import is properly structured and matches the event definition in InputHandler.ts.


531-551: LGTM: Clean toggle implementation.

The rocket direction toggle is well-implemented with proper accessibility (semantic button, translated text, aria-hidden on decorative emoji) and clean state management through the UI state.


888-895: LGTM: Close button repositioned correctly.

The absolute positioning of the close button in the panel header is a clean UI improvement with proper accessibility attributes (aria-label and title using translateText).


930-931: Toggle integration is correct and safe to use.

The rocketDirectionUp property is initialized to true in GameRenderer.ts at application startup, before any render occurs. The conditional render properly gates display to the player's own panel using other === my, and the renderRocketDirectionToggle() method is properly implemented. No risk of undefined values.

@ryanbarlow97 ryanbarlow97 changed the title add bomb mobile flip support [DRAFT] add mobile bomb flip support [DRAFT] Dec 30, 2025
@ryanbarlow97 ryanbarlow97 changed the title add mobile bomb flip support [DRAFT] add mobile bomb flip support Dec 30, 2025
@ryanbarlow97 ryanbarlow97 added UI/UX UI/UX changes including assets, menus, QoL, etc. Gameplay Features that affect gameplay Feature labels Dec 30, 2025
@ryanbarlow97 ryanbarlow97 added this to the v29 milestone Dec 30, 2025
@ryanbarlow97 ryanbarlow97 linked an issue Dec 30, 2025 that may be closed by this pull request
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/PlayerPanel.ts (1)

507-507: Fix CSS class syntax error.

Missing space between w-[5ch] and font-semibold creates an invalid class. The font weight styling won't be applied to the gold amount.

🔎 Proposed fix
-          <span translate="no" class="tabular-nums w-[5ch]font-semibold">
+          <span translate="no" class="tabular-nums w-[5ch] font-semibold">
♻️ Duplicate comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)

84-87: Redundant state update causing double render.

The event handler (lines 308-314) updates uiState.rocketDirectionUp and calls requestUpdate(), but the event subscriber (lines 84-87) performs the same operations. When a user clicks the toggle, both update the state, causing a double render.

This issue was already identified in a previous review. Consider removing the direct state update from handleToggleRocketDirection and letting the event subscriber be the single source of truth.

Based on past review comments, the fix is to remove lines 311 and 313 from the handler so it only emits the event.

Also applies to: 308-314

🧹 Nitpick comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)

737-785: Optional: Remove redundant condition check.

Line 741 checks other !== my inside a block that only executes when other !== my (due to line 737's condition). The inner check is redundant.

🔎 Proposed cleanup
         ${other === my
           ? html``
           : html`
               <div class="grid auto-cols-fr grid-flow-col gap-1">
-                ${other !== my
-                  ? canEmbargo
+                ${canEmbargo
                     ? actionButton({
                         onClick: (e: MouseEvent) =>
                           this.handleEmbargoClick(e, my, other),
                         icon: stopTradingIcon,
                         iconAlt: "Stop Trading",
                         title: translateText("player_panel.stop_trade"),
                         label: translateText("player_panel.stop_trade"),
                         type: "yellow",
                       })
                     : actionButton({
                         onClick: (e: MouseEvent) =>
                           this.handleStopEmbargoClick(e, my, other),
                         icon: startTradingIcon,
                         iconAlt: "Start Trading",
                         title: translateText("player_panel.start_trade"),
                         label: translateText("player_panel.start_trade"),
                         type: "green",
                       })
-                  : ""}
                 ${canBreakAlliance
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fb9e189 and 47cf2e3.

📒 Files selected for processing (1)
  • src/client/graphics/layers/PlayerPanel.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.

Applied to files:

  • src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.

Applied to files:

  • src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.

Applied to files:

  • src/client/graphics/layers/PlayerPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (1)
src/client/InputHandler.ts (1)
  • SwapRocketDirectionEvent (92-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Deploy to openfront.dev
🔇 Additional comments (3)
src/client/graphics/layers/PlayerPanel.ts (3)

17-21: LGTM! Import added correctly.

The SwapRocketDirectionEvent import is properly placed and necessary for the rocket direction toggle feature.


531-551: Well-structured rocket direction toggle UI.

The toggle implementation is clean and follows existing component patterns. The conditional state display and event handling are correct.


930-931: Correct conditional rendering for rocket toggle.

The toggle is correctly shown only when viewing your own panel (other === my), which aligns with the feature requirement that players control only their own rocket direction.

to update uiState and trigger re-render
coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 30, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 30, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 30, 2025
@ryanbarlow97 ryanbarlow97 marked this pull request as ready for review December 30, 2025 11:00
@ryanbarlow97 ryanbarlow97 requested a review from a team as a code owner December 30, 2025 11:00
Copilot AI review requested due to automatic review settings December 30, 2025 11:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enables mobile users to flip bomb/rocket trajectory direction through a UI button, complementing the existing keyboard shortcut. It also streamlines the player panel UI by repositioning the close button and restructuring action button layouts.

Key changes:

  • Modified SwapRocketDirectionEvent to carry the new direction state as a parameter instead of relying on implicit toggling
  • Added a rocket direction toggle button to the player panel that's visible only when viewing your own profile
  • Fixed CSS spacing issue (missing space between w-[5ch] and font-semibold)

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/client/InputHandler.ts Updated SwapRocketDirectionEvent to accept a boolean parameter; keyboard handler now passes the new direction value explicitly
src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts Modified to receive direction from event parameter instead of toggling state internally
src/client/graphics/layers/PlayerPanel.ts Added rocket direction toggle button with event handlers; repositioned close button from sticky container to absolute positioning; restructured action buttons to conditionally render based on player context; fixed CSS spacing
resources/lang/en.json Added translations for "Flip rocket trajectory", "Upward arc", and "Downward arc"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ryanbarlow97
Copy link
Contributor Author

@evanpelle would you say this is a blocker for v29? to be fairer towards mobile?

Copy link
Collaborator

@evanpelle evanpelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@evanpelle evanpelle added this pull request to the merge queue Jan 2, 2026
@evanpelle evanpelle removed this pull request from the merge queue due to a manual request Jan 2, 2026
@evanpelle evanpelle merged commit 72f8924 into main Jan 2, 2026
11 checks passed
@evanpelle evanpelle deleted the bombmobile branch January 2, 2026 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Gameplay Features that affect gameplay UI/UX UI/UX changes including assets, menus, QoL, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow mobile users to flip bomb direction

3 participants