feat: make PlayerInfoPanel available in replays#3855
feat: make PlayerInfoPanel available in replays#3855mike-s-zaugg wants to merge 2 commits intoopenfrontio:mainfrom
Conversation
Show player info (resources, alliances, stats) when clicking on a player during replay playback. Action buttons are hidden as they are not applicable in replay mode.
|
|
WalkthroughMake UI safe for replay mode: allow nullable myPlayer in radial menus, show info-only radial when no local player, and render PlayerPanel read‑only using a viewer variable while hiding interactive controls in replay. ChangesReplay Mode Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
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)
954-990:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock intent-capable modals in replay mode as well
Line 954 and Line 975 still allow modal rendering in replay when state is already set. Those modals can emit intent events, so replay is not fully read-only unless these branches are also gated by replay mode.
Suggested patch
+ const canInteract = !isReplay; ... - ${this.sendTarget + ${canInteract && this.sendTarget ? html` <send-resource-modal ... - ${this.moderationTarget + ${canInteract && this.moderationTarget ? html` <player-moderation-modal🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/graphics/layers/PlayerPanel.ts` around lines 954 - 990, PlayerPanel still renders intent-capable modals when sendTarget or moderationTarget are set even in replay mode; update the template branches that render <send-resource-modal> and <player-moderation-modal> to also check the replay flag and skip rendering when in replay. Locate the modal conditional expressions referencing this.sendTarget (and this.sendMode) and this.moderationTarget inside PlayerPanel and add a guard such as && !this.isReplay (or the project's replay flag, e.g., this.g.isReplay) so the modals are not instantiated in replay/read-only mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/graphics/layers/PlayerPanel.ts`:
- Around line 954-990: PlayerPanel still renders intent-capable modals when
sendTarget or moderationTarget are set even in replay mode; update the template
branches that render <send-resource-modal> and <player-moderation-modal> to also
check the replay flag and skip rendering when in replay. Locate the modal
conditional expressions referencing this.sendTarget (and this.sendMode) and
this.moderationTarget inside PlayerPanel and add a guard such as &&
!this.isReplay (or the project's replay flag, e.g., this.g.isReplay) so the
modals are not instantiated in replay/read-only mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38b69f60-1712-45f5-a37b-168a4f44109a
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerPanel.ts
|
Please test it locally by running npm run dev:prod You can watch all prod replays that way And a screenshot would be nice then |
|
And prettier is failing, run |
The previous commit made PlayerPanel render when myPlayer is null, but the panel is only opened from the right-click radial menu, which itself short-circuited on myPlayer === null. So the feature was unreachable in replays. - MainRadialMenu: in replay mode, skip the server actions() lookup and open the radial with empty PlayerActions on player tiles only - RadialMenuElements: widen MenuElementParams.myPlayer to PlayerView | null; root submenu returns [info] only when myPlayer is null; guard deleteUnit cooldown/disabled against null Also applies prettier to PlayerPanel.ts (CI was failing on it).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/graphics/layers/MainRadialMenu.ts`:
- Around line 118-126: The issue is that this.clickedTile is accessed inside
async callbacks in the methods around lines 118 and 188, leading to stale or
incorrect tile usage if it changes before the promise resolves. Fix this by
capturing the current clickedTile value into a local constant before calling
myPlayer.actions or other async functions, then use that local constant inside
the callback to update the player actions consistently for the intended tile.
In `@src/client/graphics/layers/PlayerPanel.ts`:
- Around line 1015-1022: renderAllianceExpiry() is still rendered when isReplay
is true, causing stale expiry text to persist because replay doesn't update the
live-only value; stop rendering or clear the underlying state when in replay:
update the template where renderAllianceExpiry() is used (near the current
conditional around renderActions) so it is not called when isReplay is true, and
additionally ensure show() and/or tick() detect entering replay mode and clear
the alliance-expiry state used by renderAllianceExpiry() (or set it to an
empty/undefined value) to be extra safe; reference renderAllianceExpiry(),
renderActions(), show(), and tick() when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 323978f8-bb58-48ca-92b2-6972e512069b
📒 Files selected for processing (3)
src/client/graphics/layers/MainRadialMenu.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/RadialMenuElements.ts
| myPlayer.actions(this.clickedTile).then((actions) => { | ||
| this.updatePlayerActions( | ||
| myPlayer, | ||
| actions, | ||
| this.clickedTile!, | ||
| event.x, | ||
| event.y, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Capture tile locally before async callbacks to avoid stale-menu updates
At Line 118 and Line 188, the callback reads this.clickedTile again after the async call. If the user right-clicks another tile before resolve, actions for tile A can be applied to tile B.
Proposed fix
- this.clickedTile = this.game.ref(worldCoords.x, worldCoords.y);
+ this.clickedTile = this.game.ref(worldCoords.x, worldCoords.y);
+ const clickedTile = this.clickedTile;
...
- myPlayer.actions(this.clickedTile).then((actions) => {
+ myPlayer.actions(clickedTile).then((actions) => {
this.updatePlayerActions(
myPlayer,
actions,
- this.clickedTile!,
+ clickedTile,
event.x,
event.y,
);
});
...
- myPlayer.actions(this.clickedTile).then((actions) => {
- this.updatePlayerActions(myPlayer, actions, this.clickedTile!);
+ const clickedTile = this.clickedTile;
+ myPlayer.actions(clickedTile).then((actions) => {
+ this.updatePlayerActions(myPlayer, actions, clickedTile);
});Also applies to: 188-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/graphics/layers/MainRadialMenu.ts` around lines 118 - 126, The
issue is that this.clickedTile is accessed inside async callbacks in the methods
around lines 118 and 188, leading to stale or incorrect tile usage if it changes
before the promise resolves. Fix this by capturing the current clickedTile value
into a local constant before calling myPlayer.actions or other async functions,
then use that local constant inside the callback to update the player actions
consistently for the intended tile.
| ${this.renderAllianceExpiry()} | ||
|
|
||
| <ui-divider></ui-divider> | ||
|
|
||
| <!-- Actions --> | ||
| ${this.renderActions(my, other)} | ||
| ${isReplay | ||
| ? "" | ||
| : html` | ||
| <ui-divider></ui-divider> | ||
| <!-- Actions --> | ||
| ${this.renderActions(viewer, other)} | ||
| `} |
There was a problem hiding this comment.
Hide or clear alliance-expiry state in replay mode
Line 1015 still renders alliance expiry in replay, but replay does not refresh that value (live-only update path), so stale text can leak from previous state.
Proposed fix
- <!-- Alliance time remaining -->
- ${this.renderAllianceExpiry()}
+ <!-- Alliance time remaining -->
+ ${isReplay ? "" : this.renderAllianceExpiry()}Optionally also clear on replay entry in show()/tick() for extra safety.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/graphics/layers/PlayerPanel.ts` around lines 1015 - 1022,
renderAllianceExpiry() is still rendered when isReplay is true, causing stale
expiry text to persist because replay doesn't update the live-only value; stop
rendering or clear the underlying state when in replay: update the template
where renderAllianceExpiry() is used (near the current conditional around
renderActions) so it is not called when isReplay is true, and additionally
ensure show() and/or tick() detect entering replay mode and clear the
alliance-expiry state used by renderAllianceExpiry() (or set it to an
empty/undefined value) to be extra safe; reference renderAllianceExpiry(),
renderActions(), show(), and tick() when making the changes.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
|
@FloPinguin I tested it localy and needed to apply some changes. I added the features and screenshots. |
|
Thank you, I think it would make sense to just show the info panel on right click, skip the info-only-radial? Yeah please check if rabbit is correct, sometimes it has good ideas / finds bugs, you can reply to its comments |
| if (myPlayer === null && !isReplay) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
so in this case we haven't spawned yet?
|
|
||
| const my = this.g.myPlayer(); | ||
| if (!my) return html``; | ||
| const isReplay = this.g.config().isReplay(); |
There was a problem hiding this comment.
I think it maybe be cleaner to create:
this.g.isSpectator()
and it just does:
isSpectator() {
!(this.myPlayer()?.isAlive) || this.config.isReplay
}
| const my = this.g.myPlayer(); | ||
| if (!my) return html``; | ||
| const isReplay = this.g.config().isReplay(); | ||
| if (!my && !isReplay) return html``; |
There was a problem hiding this comment.
so in this case we are in a live match but haven'ts spawned in? so we don't render the player panel?
|
|
||
| <!-- Actions --> | ||
| ${this.renderActions(my, other)} | ||
| ${isReplay |
There was a problem hiding this comment.
same here, instead of isReplay, it's isSpectator because being dead is the equivalent of watching a replay
4 participants
Description:
Makes the PlayerInfoPanel accessible during replay playback, as suggested by @FloPinguin.
Previously the panel returned empty immediately when
myPlayer()was null (which is always the case in replays since the viewer is not an active participant). The panel now renders in a read-only mode showing allinformational content: player name, flag, resources, alliances, betrayal count, and trading status.
The right-click radial menu was also gated on
myPlayer !== null, so it never opened in replays. In replay mode the radial now opens with only the info wedge enabled when right-clicking on a player tile, which is theentry point to the panel.
Action buttons that require sending intents to the server are hidden in replay mode since they are not applicable:
Read-only content (resources, alliances, stats) remains fully visible, which was the main motivation for this feature: being able to review alliances during a replay.
Screenshots
Radial menu only shows info option.
Displays all read-only content.
Please complete the following:
Tested locally with
npm run dev:prodagainst a real prod replay (/w1/game/tvBd246s?live).Please put your Discord username so you can be contacted if a bug or regression is found:
sxndrexe