Improve player panel #2060
Improve player panel #2060
Conversation
|
Warning Rate limit exceeded@abdallahbahrawi1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRefactors player UI, adds two UI components (ActionButton, Divider), extends English localization keys, and exposes traitor remaining ticks via Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Panel as PlayerPanel
participant Game as GameState
participant I18n as translateText
participant UI as ActionButton / Divider
participant Modal as Modal (Chat / SendTroops)
User->>Panel: open(player)
Panel->>Game: request otherProfile / subscribe
Panel->>Panel: tick() → compute allianceExpirySeconds
Panel->>I18n: translateText(player_type / player_panel keys)
I18n-->>Panel: localized strings
Panel->>UI: render action grid (actionButton, ui-divider)
Note over User,UI: User clicks an action
User->>UI: Click "Send Troops" / "Break Alliance"
UI-->>Panel: onClick handler
Panel->>Modal: open modal or invoke game action
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ 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: 1
🧹 Nitpick comments (6)
src/client/components/ui/Divider.ts (2)
6-10: Consider using stricter property types for better type safetyThe
spacingproperty would benefit from using a literal union type in the decorator itself, and thecolorproperty should have a more specific type rather than a generic string.- @property({ type: String }) - spacing: "sm" | "md" | "lg" = "md"; - - @property({ type: String }) - color: string = ""; + @property({ type: String }) + spacing: "sm" | "md" | "lg" = "md"; + + @property({ type: String }) + color: string = "bg-zinc-700/80";Also consider extracting the spacing type as a reusable type:
export type DividerSpacing = "sm" | "md" | "lg";
23-23: Inconsistent default value handlingThe default color is set using
||operator which treats empty string as falsy. This makes the empty string default on line 10 meaningless. Either use the default directly in the property declaration or handle empty strings explicitly.- const colorClass = this.color || "bg-zinc-700/80"; + const colorClass = this.color;And update the property default:
- color: string = ""; + color: string = "bg-zinc-700/80";src/client/components/ui/ActionButton.ts (2)
13-27: Consider extracting button variant type for reusabilityThe button type literals appear in multiple places. Extract them as a reusable type for better maintainability.
+export type ButtonVariant = "normal" | "red" | "green" | "indigo"; + export interface ActionButtonProps { onClick: (e: MouseEvent) => void; - type?: "normal" | "red" | "green" | "indigo"; + type?: ButtonVariant; icon: string; iconAlt: string; title: string; label: string; disabled?: boolean; }
29-31: Extract constants to module level for better organizationThese style constants are only used within the actionButton function. Consider moving them to module level for better code organization and potential reuse.
+const ICON_SIZE = "h-5 w-5 shrink-0 transition-transform group-hover:scale-110"; +const TEXT_SIZE = "text-[10px] sm:text-[11px] leading-4 font-medium tracking-tight"; -const iconSize = "h-5 w-5 shrink-0 transition-transform group-hover:scale-110"; -const textSize = - "text-[10px] sm:text-[11px] leading-4 font-medium tracking-tight"; export const actionButton = (props: ActionButtonProps): TemplateResult => { // ... rest of function - <img src=${icon} alt=${iconAlt} class="${iconSize}" /> - <span class="${textSize}">${label}</span> + <img src=${icon} alt=${iconAlt} class="${ICON_SIZE}" /> + <span class="${TEXT_SIZE}">${label}</span>src/client/graphics/layers/PlayerPanel.ts (2)
530-530: Inconsistent label for Send Troops buttonThe Send Troops button uses "player_panel.troops" for the label but "player_panel.send_troops" for the title. This creates inconsistency - the button shows just "Troops" instead of "Send Troops".
- label: translateText("player_panel.troops"), + label: translateText("player_panel.send_troops"),
545-545: Inconsistent label for Send Gold buttonSimilar to the Send Troops button, the Send Gold button shows just "Gold" instead of "Send Gold".
- label: translateText("player_panel.gold"), + label: translateText("player_panel.send_gold"),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
resources/images/StopIconWhite.pngis excluded by!**/*.pngresources/images/TradingIconWhite.pngis excluded by!**/*.png
📒 Files selected for processing (4)
resources/lang/en.json(2 hunks)src/client/components/ui/ActionButton.ts(1 hunks)src/client/components/ui/Divider.ts(1 hunks)src/client/graphics/layers/PlayerPanel.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (4)
src/client/Utils.ts (4)
renderDuration(5-13)renderNumber(19-43)translateText(92-147)renderTroops(15-17)src/core/game/GameView.ts (1)
myPlayer(509-512)src/client/components/ui/ActionButton.ts (1)
actionButton(33-57)src/core/game/PlayerImpl.ts (5)
canSendEmoji(555-573)canTarget(495-508)canDonateTroops(602-627)canDonateGold(575-600)canSendAllianceRequest(389-416)
🔇 Additional comments (6)
src/client/components/ui/ActionButton.ts (1)
3-11: Well-structured interface with clear props!The ActionButtonProps interface is clean and follows good TypeScript practices with optional properties and literal union types.
resources/lang/en.json (1)
560-584: LGTM! Comprehensive translations for player panelThe new localization keys are well-organized and cover all the UI elements mentioned in the PR objectives. The naming is consistent with existing patterns.
src/client/graphics/layers/PlayerPanel.ts (4)
222-228: Clean implementation of dynamic color coding!The alliance expiry color coding provides clear visual feedback with appropriate thresholds (30s red, 60s yellow, >60s green).
429-443: Good scrollable list implementation for alliancesThe alliance list with max-height and overflow handling prevents layout issues when players have many allies. The "None" fallback text provides clear feedback.
478-608: Excellent action button grid implementation!The new ActionButton-based layout is clean and well-organized. The conditional rendering of buttons based on permissions and the use of appropriate color coding (red for dangerous actions, green for positive) improves UX significantly. This addresses the PR objective of making risky actions safer to avoid misclicks.
8-11: Confirm icon files & fix imports in PlayerPanel.tsFound resources/images/StopIconWhite.png and resources/images/TradingIconWhite.png; presence of TargetIconWhite.svg and TraitorIconWhite.svg was not verified. Confirm that resources/images/TargetIconWhite.svg and resources/images/TraitorIconWhite.svg exist, or update the imports in src/client/graphics/layers/PlayerPanel.ts (lines 8–11) to the correct filenames/paths (or use the available .png assets).
|
The gold and troop display seems to be not vertically centered. The text "gold" is aligned slightly below the number of gold. |
I don't see a button to send troops/gold when allied? Looks good, I think the UI & text could be a bit larger. |
Y thanks |
Did you test this on team mode ? Regarding the ui/text i will Add more changes today |
abdallahbahrawi1
force-pushed
the
Improve-Player-panel
branch
from
298ea34 to
0fe923a
Compare
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/PlayerPanel.ts (1)
97-108: Normalize attackRatio units: store/use a 0–1 fractionGameRenderer initializes uiState.attackRatio as 20 (percent) while ControlPanel and consumers use 0.0–1.0; this mismatch can multiply troops by an order of magnitude.
- Fix: change src/client/graphics/GameRenderer.ts (line ~51) from
const uiState = { attackRatio: 20 }
to
const uiState = { attackRatio: 0.2 }.- Audit the send sites that multiply troops by uiState.attackRatio (src/client/graphics/layers/PlayerPanel.ts — handleDonateTroopClick, src/client/graphics/layers/PlayerActionHandler.ts, src/client/ClientGameRunner.ts) and add a defensive clamp when emitting if desired:
amount = Math.floor(myPlayer.troops() * Math.max(0, Math.min(1, uiState.attackRatio)))
♻️ Duplicate comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)
292-300: Sanitize flag path to prevent path traversal and hide broken imagesThis repeats a previously raised issue; src is built from untrusted input.
Apply:
- ? html`<img - src="/flags/${flagCode}.svg" - alt=${flagName} - class="h-10 w-10 rounded-full object-cover" - />` + ? html`<img + src=${`/flags/${encodeURIComponent(flagCode)}.svg`} + alt=${flagName || translateText("player_panel.flag")} + class="h-10 w-10 rounded-full object-cover" + loading="lazy" + decoding="async" + @error=${(e: Event) => { + (e.target as HTMLImageElement).style.display = "none"; + }} + />`
🧹 Nitpick comments (10)
resources/lang/en.json (3)
560-565: Status terms added — consider grouping for reuseOptional: nest under player_panel.status.* (e.g., status.trust, status.trading.active/stopped) to keep keys consistent and easier to translate across screens.
574-585: Tidy up action keys and add UX hints
- Key player_panel.break looks redundant next to player_panel.break_alliance; remove or standardize.
- Add helper strings to explain why actions are hidden/disabled (e.g., team-mode only):
[sendable suggestions]+ "team_mode_only": "Team mode only", + "ffa_public_donation_disabled": "Donations disabled in public FFA",
587-589: Copy nit: Title case for “Max”To match surrounding labels, prefer “Max”.
- "fastest_game_speed": "max" + "fastest_game_speed": "Max"src/client/graphics/layers/PlayerPanel.ts (7)
197-216: Alliance expiry can get stuck after expirationWhen remainingTicks <= 0, you keep showing the last value. Clear both states.
- if (remainingTicks > 0) { - const remainingSeconds = Math.max( - 0, - Math.floor(remainingTicks / 10), - ); // 10 ticks per second - this.allianceExpirySeconds = remainingSeconds; - this.allianceExpiryText = renderDuration(remainingSeconds); - } + const secs = Math.max(0, Math.floor(remainingTicks / 10)); // 10 ticks per second + if (remainingTicks > 0) { + this.allianceExpirySeconds = secs; + this.allianceExpiryText = renderDuration(secs); + } else { + this.allianceExpirySeconds = null; + this.allianceExpiryText = null; + }
310-341: Fix resource chip vertical alignment (matches the reported UI issue)Align emoji, number, and label; use tabular numerals for steadier layout.
- <div class="mb-1 flex justify-between gap-2"> + <div class="mb-1 flex justify-between gap-2"> <div - class="inline-flex items-center gap-0.5 rounded-full bg-zinc-800 px-2.5 py-1 + class="inline-flex items-baseline gap-1 rounded-full bg-zinc-800 px-2.5 py-1 text-sm font-medium text-zinc-200" > <span>💰</span> <span translate="no" - class="inline-block w-[40px] text-right" + class="inline-block w-[44px] text-right tabular-nums leading-none" > ${renderNumber(other.gold() || 0)} </span> - <span class="opacity-90"> + <span class="opacity-90 leading-none"> ${translateText("player_panel.gold")} </span> </div> <div - class="inline-flex items-center gap-1 rounded-full bg-zinc-800 px-2.5 py-1 + class="inline-flex items-baseline gap-1 rounded-full bg-zinc-800 px-2.5 py-1 text-sm font-medium text-zinc-200" > <span>🛡️</span> <span translate="no" - class="inline-block w-[40px] text-right" + class="inline-block w-[44px] text-right tabular-nums leading-none" > ${renderTroops(other.troops() || 0)} </span> - <span class="opacity-90"> + <span class="opacity-90 leading-none"> ${translateText("player_panel.troops")} </span> </div> </div>
162-165: Stop event bubbling when opening chatPrevents side-effects from global listeners.
- private handleChat(e: Event, sender: PlayerView, other: PlayerView) { + private handleChat(e: Event, sender: PlayerView, other: PlayerView) { + e.stopPropagation(); this.ctModal.open(sender, other); this.hide(); }
179-186: Duplicate CloseViewEvent subscriptionYou subscribe in both initEventBus and init; hide() will fire twice.
init() { - this.eventBus.on(MouseUpEvent, () => this.hide()); - this.eventBus.on(CloseViewEvent, (e) => { - this.hide(); - }); + this.eventBus.on(MouseUpEvent, () => this.hide());Also applies to: 190-193
235-238: Return an empty template in early exitsKeep render type consistent.
- const myPlayer = this.g.myPlayer(); - if (myPlayer === null) return; - if (this.tile === null) return; + const myPlayer = this.g.myPlayer(); + if (myPlayer === null) return html``; + if (this.tile === null) return html``;
521-549: Button labels: use “Send …” for clarityMatch titles for consistency.
- title: translateText("player_panel.send_troops"), - label: translateText("player_panel.troops"), + title: translateText("player_panel.send_troops"), + label: translateText("player_panel.send_troops"), ... - title: translateText("player_panel.send_gold"), - label: translateText("player_panel.gold"), + title: translateText("player_panel.send_gold"), + label: translateText("player_panel.send_gold"),
481-490: Consider disabled buttons with tooltips instead of hiding actionsPlayers reported not seeing donate/send actions. Render the button disabled with a reason (e.g., “Team mode only”) to reduce confusion. Needs small prop extension on actionButton for tooltip text.
Also applies to: 506-520, 521-549, 552-607
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.json(2 hunks)src/client/graphics/layers/PlayerPanel.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T12:03:08.052Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1655
File: resources/maps/giantworldmap/manifest.json:165-174
Timestamp: 2025-07-31T12:03:08.052Z
Learning: In OpenFrontIO draft PRs, flag names in map manifest files may be placeholders that will be updated to match the actual SVG flag file names when the assets are received. The final naming depends on the actual flag SVG files provided, not code naming conventions.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (4)
src/client/Utils.ts (4)
renderDuration(5-13)renderNumber(19-43)translateText(92-147)renderTroops(15-17)src/core/game/GameView.ts (1)
myPlayer(509-512)src/client/components/ui/ActionButton.ts (1)
actionButton(33-57)src/core/game/PlayerImpl.ts (5)
canSendEmoji(555-573)canTarget(495-508)canDonateTroops(602-627)canDonateGold(575-600)canSendAllianceRequest(389-416)
🔇 Additional comments (2)
resources/lang/en.json (1)
558-558: LGTM: concise label for BetrayalsMatches the new PlayerPanel usage.
src/client/graphics/layers/PlayerPanel.ts (1)
222-229: LGTM: clear, readable mapping from seconds → colorSolid thresholds and simple defaults.
This is how it looks right now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/client/components/ui/Divider.ts (3)
18-28: Harden spacing handling and tighten typingIndexing
spacingClasses[this.spacing]can yieldundefinedif an invalid value is passed. Add a typed map and a safe fallback.- const spacingClasses = { - sm: "my-0.5", - md: "my-1", - lg: "my-2", - }; + const spacingClasses: Record<DividerSpacing, string> = { + sm: "my-0.5", + md: "my-1", + lg: "my-2", + } as const; + const spacing = spacingClasses[this.spacing] ?? spacingClasses.md; @@ - return html`<div - class="${spacingClasses[this.spacing]} h-px ${colorClass}" - ></div>`; + return html`<div + class="${spacing} h-px ${colorClass}" + ></div>`;
26-28: Add a11y semantics to decorative ruleExpose semantics for assistive tech. Keep the
divbut mark it as a visual separator.- return html`<div - class="${spacing} h-px ${colorClass}" - ></div>`; + return html`<div + role="separator" + aria-hidden="true" + class="${spacing} h-px ${colorClass}" + ></div>`;
26-28: Minor: considerclassMapfor safer class compositionOptional: using
classMapfromlit/directives/class-map.jsavoids string concatenation bugs and makes conditional classes clearer.resources/lang/en.json (4)
560-565: Trading status copy: align with issue #2015 (“Active/Disabled”)The issue text calls for “Trading: Active/Disabled”. Current value is “Stopped”. Suggest “Disabled” for consistency.
"trading": "Trading", "active": "Active", - "stopped": "Stopped", + "stopped": "Disabled",
558-565: Trust status terms: add “Betrayer” label to pair with “Stable”Objectives mention “Trust: Stable/Betrayer”. We have “stable” but not a matching “betrayer”. Add the key so UI can render both states cleanly. If code currently uses “traitor”, decide on one term and stick to it.
"betrayals": "Betrayals", "traitor": "Traitor", "stable": "Stable", + "betrayer": "Betrayer", "trust": "Trust",Follow‑up: if you prefer “Traitor” everywhere, remove “betrayer” and use “traitor” in the trust line too.
574-584: Action labels: clarify and unify wording
- “Send Alliance” reads oddly; “Request Alliance” is clearer.
- “Emotes” vs “emoji” is inconsistent with
help_modal.info_emoji. Prefer “Emojis”.- "send_alliance": "Send Alliance", + "send_alliance": "Request Alliance", "send_troops": "Send Troops", "send_gold": "Send Gold", - "emotes": "Emotes" + "emotes": "Emojis"
577-579: Drop the unused "player_panel.break" translation keyOnly "player_panel.break_alliance" is referenced in code (src/client/graphics/layers/PlayerPanel.ts:585–586); remove "break": "Break" from resources/lang/en.json (lines 576–579) or keep it only if a different, generic "Break" UI is intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.json(1 hunks)src/client/components/ui/ActionButton.ts(1 hunks)src/client/components/ui/Divider.ts(1 hunks)src/client/graphics/layers/PlayerPanel.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/components/ui/ActionButton.ts
- src/client/graphics/layers/PlayerPanel.ts
🔇 Additional comments (5)
src/client/components/ui/Divider.ts (3)
13-15: Light DOM rendering: confirm intent
createRenderRoot() { return this; }opts out of shadow DOM. This is fine if you want Tailwind utilities from the page to style internals. Confirm this is intentional for consistency with other UI components.
4-12: Public API looks cleanGood use of a typed union for spacing and a simple color API. Defaults are sensible.
1-1: Heads-up: one PlayerPanel usage seems malformedIn src/client/graphics/layers/PlayerPanel.ts (~150–220), one instance shows
<ui-divider></ui-dividermissing the closing>. Please fix to avoid template parse errors.resources/lang/en.json (2)
568-569: Trade buttons: LGTMLabels are clear and match the control’s behavior.
588-588: “Max” vs prior “Fastest”Changing to “Max” is fine if other speed labels (e.g., “Min/Max”) use the same pattern. Please confirm consistency across UI and tutorials.
There was a problem hiding this comment.
Actionable comments posted: 3
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)
103-110: Clamp and round donation amount to a safe integer ≥ 1.Prevents sending fractional or zero troops.
Apply:
- this.eventBus.emit( - new SendDonateTroopsIntentEvent( - other, - myPlayer.troops() * this.uiState.attackRatio, - ), - ); + const donateAmount = Math.max( + 1, + Math.floor(myPlayer.troops() * this.uiState.attackRatio), + ); + this.eventBus.emit(new SendDonateTroopsIntentEvent(other, donateAmount));
🧹 Nitpick comments (7)
src/client/graphics/layers/PlayerPanel.ts (6)
312-326: Fix vertical alignment of “Gold”/“Troops” label and value.Reported misalignment likely due to center alignment; baseline alignment reads better.
Apply:
- <div - class="inline-flex items-center gap-1.5 rounded-full bg-zinc-800 px-2.5 py-1 - text-lg font-semibold text-zinc-100" - > + <div + class="inline-flex items-baseline gap-1.5 rounded-full bg-zinc-800 px-2.5 py-1 + text-lg font-semibold text-zinc-100" + > ... - <div - class="inline-flex items-center gap-1.5 rounded-full bg-zinc-800 px-2.5 py-1 - text-lg font-semibold text-zinc-100" - > + <div + class="inline-flex items-baseline gap-1.5 rounded-full bg-zinc-800 px-2.5 py-1 + text-lg font-semibold text-zinc-100" + >Also applies to: 328-342
276-278: Remove stray “order” class.Likely a typo; Tailwind expects order-1/order-first/etc. Harmless but noisy.
- class="relative mt-2 w-full order border-white/10 bg-zinc-900/95 + class="relative mt-2 w-full border border-white/10 bg-zinc-900/95 backdrop-blur-sm p-5 shadow-2xl ring-1 ring-zinc-800 rounded-xl text-zinc-200"
510-535: Send Troops/Gold actions are one‑tap; consider modal per issue #2015.Current UX sends immediately using attack ratio or “null” amount for gold. Issue #2015 asks for a dedicated modal with presets and safety options.
I can scaffold a small modal with presets (10/25/50/75), slider, %/Exact toggle, and “Auto‑keep ≥30%” checkbox, wired to SendDonateTroopsIntentEvent/SendDonateGoldIntentEvent. Want me to draft it?
288-305: Alt text and name rendering: escape and fallback.If a country name is missing, use a sane fallback for alt. Also consider trimming/escaping player names (Lit escapes by default; keep translate="no" off the name).
You already set alt via country name; the flagCode fix above adds a fallback. No extra change needed here beyond that.
471-486: a11y: Add aria-label to close button.Improve screen reader support.
- <button + <button @click=${this.handleClose} class="absolute -top-3 -right-3 flex h-7 w-7 items-center justify-center rounded-full bg-zinc-700 text-white shadow hover:bg-red-500 transition-colors" + aria-label="${translateText('common.close')}" >
203-211: Extract tick-to-second rate into shared constant
DefineTICKS_PER_SECONDin config or a constants file and replace/10conversions inPlayerPanel.ts:203andPlayerInfoOverlay.ts:216withremainingTicks / TICKS_PER_SECOND.src/client/components/ui/ActionButton.ts (1)
26-31: Style disabled state for better UX.Make disabled visible and non-interactive.
const btnBase = "group w-full min-w-[50px] select-none flex flex-col items-center justify-center " + "gap-1 rounded-lg py-1.5 border border-white/10 bg-white/[0.04] shadow-sm " + - "transition-all duration-150 " + + "transition-all duration-150 disabled:opacity-50 disabled:cursor-not-allowed " + "focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white/20 " + "active:translate-y-[1px]";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.json(1 hunks)src/client/components/ui/ActionButton.ts(1 hunks)src/client/components/ui/Divider.ts(1 hunks)src/client/graphics/layers/PlayerPanel.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/components/ui/Divider.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T12:03:08.052Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1655
File: resources/maps/giantworldmap/manifest.json:165-174
Timestamp: 2025-07-31T12:03:08.052Z
Learning: In OpenFrontIO draft PRs, flag names in map manifest files may be placeholders that will be updated to match the actual SVG flag file names when the assets are received. The final naming depends on the actual flag SVG files provided, not code naming conventions.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (4)
src/client/Utils.ts (4)
renderDuration(5-13)renderNumber(19-43)translateText(92-147)renderTroops(15-17)src/core/game/GameView.ts (1)
myPlayer(509-512)src/client/components/ui/ActionButton.ts (1)
actionButton(43-67)src/core/game/PlayerImpl.ts (5)
canSendEmoji(555-573)canTarget(495-508)canDonateTroops(602-627)canDonateGold(575-600)canSendAllianceRequest(389-416)
🔇 Additional comments (4)
src/client/graphics/layers/PlayerPanel.ts (2)
539-559: Verify embargo toggle semantics match server (“start” begins embargo, “stop” ends it).UI shows “Stop Trading” and calls handleEmbargoClick → SendEmbargoIntentEvent(..., "start"), and “Start Trading” calls "stop". That seems correct if "start" = start embargo. Given an earlier review flagged inversion, please re‑verify against server handling.
Run:
#!/bin/bash # Inspect embargo intent handling and label->action mapping rg -nP -C3 --type=ts 'SendEmbargoIntentEvent|embargo|stopEmbargo|addEmbargo|startEmbargo|start_trade|stop_trade'Expected:
- “Stop Trading” → action "start" (begin embargo).
- “Start Trading” → action "stop" (end embargo).
If reversed in server code, swap the handlers here accordingly.Also applies to: 122-140
293-299: Sanitize flagCode before building the image URL (path traversal/XSS risk).User-controlled flagCode is interpolated into the src URL without validation. Encode and strictly validate to avoid malformed paths and potential injection. Also add a safe alt fallback and hide the image on load error.
Apply these diffs:
- const flagCode = other.cosmetics.flag; - const country = - typeof flagCode === "string" - ? Countries.find((c) => c.code === flagCode) - : undefined; - const flagName = country?.name; + const flagCode = other.cosmetics.flag; + const safeFlagCode = + typeof flagCode === "string" && /^[a-z0-9_-]+$/i.test(flagCode) + ? flagCode + : null; + const country = + typeof safeFlagCode === "string" + ? Countries.find((c) => c.code === safeFlagCode) + : undefined; + const flagName = country?.name;- ${country - ? html`<img - src="/flags/${flagCode}.svg" - alt=${flagName} - class="h-10 w-10 rounded-full object-cover" - />` - : ""} + ${safeFlagCode + ? html`<img + src="/flags/${encodeURIComponent(safeFlagCode)}.svg" + alt=${flagName || translateText("player_panel.flag")} + class="h-10 w-10 rounded-full object-cover" + @error=${(e: Event) => { + (e.target as HTMLImageElement).style.display = "none"; + }} + />` + : ""}Also applies to: 258-264
resources/lang/en.json (2)
589-589: Replay label casing change is fine.“Max” matches UI conventions elsewhere.
556-584: LGTM: player_panel translation keys present. All keys used in PlayerPanel.ts exist in resources/lang/en.json and match casing.
…ons, replace trust label with traitor badge on alliance break, and add soft glowing ring animation for traitors
Last push changes |
There was a problem hiding this comment.
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)
212-220: Don’t pass null to SendDonateGoldIntentEvent — it throwsPassing null causes a runtime error. Prompt for an amount and send a valid Gold (bigint). Clamp to available balance.
private handleDonateGoldClick( e: Event, myPlayer: PlayerView, other: PlayerView, ) { e.stopPropagation(); - this.eventBus.emit(new SendDonateGoldIntentEvent(other, null)); + const input = window.prompt("Enter gold amount to send:"); + if (input === null) return; + const trimmed = input.trim(); + if (!/^\d+$/.test(trimmed)) return; + const requested = BigInt(trimmed); + if (requested <= 0n) return; + const available = myPlayer.gold(); + const toSend = requested > available ? available : requested; + this.eventBus.emit(new SendDonateGoldIntentEvent(other, toSend)); this.hide(); }
🧹 Nitpick comments (8)
src/core/game/GameView.ts (1)
348-350: Expose traitor ticks via PlayerView — consider naming cleanupMethod works. Suggest renaming the data field to a noun (e.g., traitorRemainingTicks) to match other PlayerUpdate fields and avoid “get*” on plain data.
Apply if you rename the field across types:
- getTraitorRemainingTicks(): number { - return this.data.getTraitorRemainingTicks; - } + getTraitorRemainingTicks(): number { + return this.data.traitorRemainingTicks; + }src/core/game/PlayerImpl.ts (1)
146-147: Wire-up is correct; prefer a noun field name in updatesGood addition. Recommend renaming the serialized field to traitorRemainingTicks for consistency with other PlayerUpdate fields.
- getTraitorRemainingTicks: this.getTraitorRemainingTicks(), + traitorRemainingTicks: this.getTraitorRemainingTicks(),src/core/game/GameUpdates.ts (1)
163-164: Prefer a noun for the field namePlayerUpdate should avoid “get*” for data fields. Suggest traitorRemainingTicks: number for clarity and to align with other update payloads.
- getTraitorRemainingTicks: number; + traitorRemainingTicks: number;src/client/graphics/layers/PlayerPanel.ts (5)
353-357: Use the PlayerView API instead of reaching into .dataPrefer calling player.getTraitorRemainingTicks() for encapsulation.
- const ticksLeft = player.data.getTraitorRemainingTicks ?? 0; + const ticksLeft = player.getTraitorRemainingTicks?.() ?? 0;
205-208: Send an integer number of troopsEnsure we don’t send fractional troops. Floor and clamp to at least 1 (if any troops).
- other, - myPlayer.troops() * this.uiState.attackRatio, + other, + Math.max(1, Math.floor(myPlayer.troops() * this.uiState.attackRatio)),
721-725: Tighten the isPlayer checkThe function reference check is redundant. This is clearer.
- if (!owner || !owner.isPlayer || !owner.isPlayer()) { + if (!owner || !owner.isPlayer()) { this.hide(); console.warn("Tile is not owned by a player"); return html``; }
773-777: Add accessible label to the close buttonImprove accessibility for screen readers.
- <button + <button @click=${this.handleClose} class="absolute -top-3 -right-3 flex h-7 w-7 items-center justify-center rounded-full bg-zinc-700 text-white shadow hover:bg-red-500 transition-colors" + aria-label=${translateText("player_panel.close") || "Close"} + title=${translateText("player_panel.close") || "Close"} >
136-144: Replace magic “10” with sharedTICKS_PER_SECONDconstant
Extractexport const TICKS_PER_SECOND = 10into a central utils/config module and import it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
resources/images/TraitorIconLightRed.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
resources/lang/en.json(2 hunks)src/client/graphics/layers/PlayerInfoOverlay.ts(1 hunks)src/client/graphics/layers/PlayerPanel.ts(5 hunks)src/core/game/GameUpdates.ts(1 hunks)src/core/game/GameView.ts(1 hunks)src/core/game/PlayerImpl.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#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-07-31T12:03:08.052Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1655
File: resources/maps/giantworldmap/manifest.json:165-174
Timestamp: 2025-07-31T12:03:08.052Z
Learning: In OpenFrontIO draft PRs, flag names in map manifest files may be placeholders that will be updated to match the actual SVG flag file names when the assets are received. The final naming depends on the actual flag SVG files provided, not code naming conventions.
Applied to files:
src/client/graphics/layers/PlayerPanel.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#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
🧬 Code graph analysis (2)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
src/client/Utils.ts (1)
translateText(92-147)
src/client/graphics/layers/PlayerPanel.ts (9)
src/client/components/ui/Divider.ts (1)
customElement(5-33)src/client/graphics/layers/ChatModal.ts (1)
customElement(22-329)src/client/graphics/layers/EmojiTable.ts (1)
customElement(12-123)src/core/game/GameView.ts (10)
GameView(367-699)owner(108-110)owner(557-559)PlayerView(180-365)id(85-87)id(263-265)myPlayer(512-515)player(517-523)name(249-253)allies(281-285)src/core/EventBus.ts (1)
EventBus(7-44)src/client/graphics/UIState.ts (1)
UIState(1-3)src/core/game/Game.ts (2)
PlayerActions(716-721)PlayerProfile(731-734)src/client/Utils.ts (4)
renderDuration(5-13)translateText(92-147)renderNumber(19-43)renderTroops(15-17)src/client/components/ui/ActionButton.ts (1)
actionButton(43-67)
🔇 Additional comments (5)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
269-279: Nice reuse of the new player_type keys.Swapping to
player_type.*keeps the overlay in sync with the shared translation namespace and reduces duplicated strings. Looks good to me.src/core/game/PlayerImpl.ts (1)
423-433: Traitor state computation looks correctThe logic is clear, bounded, and reuses config().traitorDuration(). No issues.
src/client/graphics/layers/PlayerPanel.ts (3)
452-458: Sanitize flag image path to prevent injection/path traversalThis was flagged earlier and still applies. Encode the flagCode and add a safe alt + error handler.
- ? html`<img - src="/flags/${flagCode}.svg" - alt=${country?.name} - class="h-10 w-10 rounded-full object-cover" - />` + ? html`<img + src="/flags/${encodeURIComponent(flagCode as string)}.svg" + alt=${country?.name || "Flag"} + class="h-10 w-10 rounded-full object-cover" + @error=${(e: Event) => { + (e.target as HTMLImageElement).style.display = "none"; + }} + />`
106-110: Guard ChatModal lookups to avoid runtime errorsIf is missing, open() will throw. Add a null check and warn.
init() { this.eventBus.on(MouseUpEvent, () => this.hide()); - this.ctModal = document.querySelector("chat-modal") as ChatModal; + this.ctModal = document.querySelector("chat-modal") as ChatModal; + if (!this.ctModal) { + console.warn("ChatModal element not found in DOM"); + } }private handleChat(e: Event, sender: PlayerView, other: PlayerView) { e.stopPropagation(); - this.ctModal.open(sender, other); - this.hide(); + if (!this.ctModal) { + console.warn("ChatModal element not found in DOM"); + return; + } + this.ctModal.open(sender, other); + this.hide(); }Also applies to: 262-266
129-148: Resolve: allianceExpiresAt exists on PlayerInteraction
PlayerInteraction already declaresallianceExpiresAt?: Tick, and your!== undefinedcheck safely handles its optionality. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/components/ui/ActionButton.ts(1 hunks)src/client/graphics/layers/PlayerPanel.ts(5 hunks)src/core/game/GameUpdates.ts(1 hunks)src/core/game/GameView.ts(1 hunks)src/core/game/PlayerImpl.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/game/PlayerImpl.ts
- src/client/graphics/layers/PlayerPanel.ts
- src/client/components/ui/ActionButton.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/game/GameView.ts (1)
407-409: Safe default looks good; consider clamping and an idiomatic getter.
- Clamp to non-negative so UI never shows negative ticks.
- Optional: expose as a TS getter property for cleaner call sites.
Suggested tweak:
- getTraitorRemainingTicks(): number { - return this.data.traitorRemainingTicks ?? 0; - } + getTraitorRemainingTicks(): number { + return Math.max(0, this.data.traitorRemainingTicks ?? 0); + }Or as a property:
get traitorRemainingTicks(): number { return Math.max(0, this.data.traitorRemainingTicks ?? 0); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/game/GameUpdates.ts(1 hunks)src/core/game/GameView.ts(1 hunks)
🔇 Additional comments (1)
src/core/game/GameUpdates.ts (1)
163-163: Good change: make traitorRemainingTicks optional.This aligns the wire contract with reality and avoids NaN/undefined surprises in consumers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/client/components/ui/ActionButton.ts (1)
43-69: LGTM — Accessibility improvements applied correctly.The
type="button",aria-label="${title}", andaria-hidden="true"on the icon have been applied as suggested in the previous review. The component now prevents accidental form submits and properly hides decorative icons from assistive tech.src/client/graphics/layers/PlayerPanel.ts (3)
184-192: Critical: Passing null to SendDonateGoldIntentEvent causes runtime error.This issue was flagged in a previous review but remains unresolved.
DonateGoldExecutionthrows whengold === null, causing runtime errors.Prompt the user for a gold amount before emitting the event. You can use a modal or input dialog:
private handleDonateGoldClick( e: Event, myPlayer: PlayerView, other: PlayerView, ) { e.stopPropagation(); - this.eventBus.emit(new SendDonateGoldIntentEvent(other, null)); - this.hide(); + // TODO: Open gold donation modal/prompt to get amount + // For now, donate a fixed amount (e.g., 100 gold) or myPlayer.gold() / 2 + const amount = Math.floor(myPlayer.gold() / 2); // donate half + this.eventBus.emit(new SendDonateGoldIntentEvent(other, amount)); + this.hide(); }Note: The PR objectives mention "add a modal for sending gold" as a future plan. Consider opening an issue to track this work if not already planned.
408-414: Sanitize flag path to prevent path traversal.The flag SVG path is constructed directly from
flagCodewithout validation. IfflagCodecontains../or other special characters, it could enable path traversal or reference unintended resources.Validate
flagCodeagainst a whitelist (e.g., theCountriesarray) before building the path:private renderIdentityRow(other: PlayerView, my: PlayerView) { const flagCode = other.cosmetics.flag; const country = typeof flagCode === "string" ? Countries.find((c) => c.code === flagCode) : undefined; + // Only render flag if validated + const safeFlagCode = country?.code; const chip = ... return html` <div class="flex items-center gap-2.5 flex-wrap"> - ${country + ${safeFlagCode ? html`<img - src="/flags/${flagCode}.svg" + src="/flags/${safeFlagCode}.svg" alt=${country?.name} class="h-10 w-10 rounded-full object-cover" + @error=${(e: Event) => { + (e.target as HTMLImageElement).style.display = 'none'; + }} />` : ""} ... </div> ... `; }This ensures only validated country codes from
Countries.jsonare used in the path.
252-262: Remove unused getIdentityKind methodThe helper isn’t referenced anywhere and can be removed to reduce dead code:
- private getIdentityKind(p: PlayerView): "nation" | "bot" | "player" { - switch (p.type()) { - case PlayerType.FakeHuman: - return "nation"; - case PlayerType.Bot: - return "bot"; - case PlayerType.Human: - default: - return "player"; - } - }
🧹 Nitpick comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)
696-721: Traitor ring animation enhances clarity.The glowing pulse animation for traitors is a strong visual cue that aligns with the PR objective to improve clarity. The animation is subtle (2.4s ease-in-out) and doesn't distract.
Consider adding a note in the CSS comment explaining why this style exists (e.g., "Traitor emphasis per issue #2015") for future maintainers.
Add a brief rationale comment:
<style> - /* Soft glowing ring animation for traitors */ + /* Soft glowing ring animation for traitors (issue #2015: improve clarity for risky states) */ .traitor-ring { ... } ... </style>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/components/ui/ActionButton.ts(1 hunks)src/client/graphics/layers/PlayerPanel.ts(6 hunks)src/core/game/GameView.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/game/GameView.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (6)
src/core/game/Game.ts (1)
PlayerProfile(738-741)src/core/EventBus.ts (1)
EventBus(7-44)src/client/InputHandler.ts (2)
CloseViewEvent(71-71)MouseUpEvent(7-12)src/core/game/GameView.ts (8)
PlayerView(177-424)id(82-84)id(322-324)type(90-92)type(328-330)player(575-581)name(308-312)allies(340-344)src/client/Utils.ts (4)
renderDuration(5-13)translateText(92-147)renderNumber(19-43)renderTroops(15-17)src/client/components/ui/ActionButton.ts (1)
actionButton(43-69)
🔇 Additional comments (9)
src/client/graphics/layers/PlayerPanel.ts (9)
94-130: Verify tick method race conditions and error handling.The tick method fetches profile and actions on every tick when visible. A few concerns:
- Race condition: If
this.tileor the owner changes between checks,_profileForPlayerIdtracking may desync (e.g., tile switches owners mid-tick).- No error handling:
pv.profile()andmyPlayer.actions(this.tile)are async and can throw, but errors aren't caught.- Potential performance: Fetching actions every tick (10 times per second) may be excessive—consider debouncing or caching with TTL.
Add error handling and consider edge cases:
async tick() { if (this.isVisible && this.tile) { + try { const owner = this.g.owner(this.tile); if (owner && owner.isPlayer()) { const pv = owner as PlayerView; const id = pv.id(); - // fetch only if we don't have it or the player changed if (this._profileForPlayerId !== Number(id)) { this.otherProfile = await pv.profile(); this._profileForPlayerId = Number(id); } } - // Refresh actions & alliance expiry const myPlayer = this.g.myPlayer(); if (myPlayer !== null && myPlayer.isAlive()) { this.actions = await myPlayer.actions(this.tile); if (this.actions?.interaction?.allianceExpiresAt !== undefined) { const expiresAt = this.actions.interaction.allianceExpiresAt; const remainingTicks = expiresAt - this.g.ticks(); - const remainingSeconds = Math.max(0, Math.floor(remainingTicks / 10)); // 10 ticks per second + const remainingSeconds = Math.max(0, Math.floor(remainingTicks / 10)); if (remainingTicks > 0) { this.allianceExpirySeconds = remainingSeconds; this.allianceExpiryText = renderDuration(remainingSeconds); } else { this.allianceExpirySeconds = null; this.allianceExpiryText = null; } } else { this.allianceExpirySeconds = null; this.allianceExpiryText = null; } this.requestUpdate(); } + } catch (err) { + console.error("PlayerPanel tick error:", err); + this.hide(); + } } }Additionally, verify with the maintainer whether fetching actions every tick is intentional or if caching with a TTL (e.g., refresh every 1-2 seconds) would be acceptable.
330-334: Traitor remaining seconds calculation uses Math.ceil — verify intent.The method uses
Math.ceil(ticksLeft / 10)to convert ticks to seconds, rounding up. This means:
- 1-10 ticks → 1 second
- 11-20 ticks → 2 seconds
Confirm this is the desired behavior. If you want to show the exact remaining time (rounding down), use
Math.floorinstead. If rounding up is intentional to avoid showing "0s" prematurely, this is correct.
336-374: Traitor badge rendering looks good — accessible and well-structured.The badge uses
role="status",aria-live="polite", andaria-atomic="true"for accessibility. The conditional pulse animation for low remaining time is a nice touch.One minor note: the dot separator uses a conditional class with
animate-pulseonly whensecs <= 10. This is intentional emphasis for urgency, which aligns with the PR objectives to improve clarity.
376-392: Nation relation pill logic is sound.The method correctly:
- Returns empty if not a Nation (
FakeHuman)- Hides the pill if traitor or allied (avoiding redundant info)
- Fetches relation from cached
otherProfileThe fallback to
Relation.Neutralis appropriate when no relation data exists.
438-542: Resource and stats rendering looks clean.The use of
renderNumber,renderTroops, andtranslateTextensures consistent formatting and i18n. The alliances list scrolls with a max height, which is good UX for large ally lists.The
translate="no"attribute on numeric/name displays is appropriate to prevent translation tools from mangling data.
544-561: Alliance expiry color logic is clear and intuitive.The thresholds (red ≤30s, yellow ≤60s, green >60s) provide good visual feedback for urgency. The use of
inline-flexwith rounded-full styling gives a polished chip appearance.
723-729: Backdrop click-to-close behavior is correct.The outer
divat line 728 has@click=${() => this.hide()}, and the inner panel at line 733 has@click=${(e: MouseEvent) => e.stopPropagation()}. This ensures clicking the backdrop closes the panel, but clicking inside does not.The backdrop styling (
bg-black/40 backdrop-blur-sm) provides good visual separation from the game.
415-417: Ignore XSS warning: Lit’s default text binding escapes HTML.${other.name()}inside the<h1>is rendered as plain text and cannot introduce unescaped HTML.Likely an incorrect or invalid review comment.
631-652: Embargo toggle logic is correct.canEmbargois set to!player.hasEmbargoAgainst(other), so showing “Stop Trading” to emit"start"and “Start Trading” to emit"stop"aligns with server semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/client/graphics/layers/PlayerPanel.ts (2)
184-192: Critical: SendDonateGoldIntentEvent still passes null.Per past review, passing
nulltoSendDonateGoldIntentEventcausesDonateGoldExecutionto throw. Prompt the user for a donation amount instead of passingnull.This issue was flagged in a previous review but remains unresolved. The user must be prompted for a valid
Goldvalue before emitting the event.
382-424: XSS risk: flag path not validated.Lines 398-401 construct the flag SVG path directly from
flagCodewithout validation, which can enable path traversal or XSS. RestrictflagCodeto a whitelist or strict regex (e.g., ISO codes, lowercase letters/dashes), and sanitizecountry?.namebefore use inalt.Apply this diff:
${country ? html`<img - src="/flags/${flagCode}.svg" - alt=${country?.name} + src="/flags/${encodeURIComponent(flagCode)}.svg" + alt=${country?.name || "Flag"} class="h-10 w-10 rounded-full object-cover" + @error=${(e: Event) => { + (e.target as HTMLImageElement).style.display = 'none'; + }} />` : ""}Additionally, consider validating
flagCodeagainst a whitelist of known country codes before constructing the path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerPanel.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerPanel.ts (6)
src/core/game/Game.ts (1)
PlayerProfile(740-743)src/core/EventBus.ts (1)
EventBus(7-44)src/client/InputHandler.ts (2)
CloseViewEvent(72-72)MouseUpEvent(8-13)src/core/game/GameView.ts (8)
PlayerView(177-424)id(82-84)id(322-324)type(90-92)type(328-330)player(575-581)name(308-312)allies(340-344)src/client/Utils.ts (4)
renderDuration(5-13)translateText(92-147)renderNumber(19-43)renderTroops(15-17)src/client/components/ui/ActionButton.ts (1)
actionButton(43-69)
🔇 Additional comments (12)
src/client/graphics/layers/PlayerPanel.ts (12)
1-46: Imports look clean.The new dependencies (ActionButton, Divider, utility functions, and icons) are well-organized and appropriate for the refactored UI.
48-71: State properties look good.The new state properties (
allianceExpiryText,allianceExpirySeconds,otherProfile,_profileForPlayerId) andctModalreference are well-typed and support the alliance expiry countdown and profile fetching features.
94-130: Tick logic is efficient and correct.The async
tick()method efficiently fetches the player profile only when the observed player changes (line 101), and correctly computes alliance expiry in seconds (line 114:remainingTicks / 10).
252-277: identityChipProps is clean.The mapping from
PlayerTypeto chip configuration (label, aria, classes, icon) is clear and consistent.
279-309: Relation mapping helpers are clean.
getRelationClassandgetRelationNameprovide consistent styling and translated labels for relation states.
311-322: Expiry and traitor helpers are correct.
getExpiryColorClassprovides clear visual feedback (red <= 30s, yellow <= 60s, green otherwise).getTraitorRemainingSecondscorrectly converts ticks to seconds (line 321).
324-362: Traitor badge is well-designed.The badge includes a countdown, pulsing animation for urgency (line 331), and accessibility attributes (aria-live, aria-atomic). Clean implementation.
364-380: Relation pill guards are correct.The function guards against non-Nation players (line 365), traitors (line 366), and allies (line 367), ensuring the relation pill is only shown when relevant.
426-496: Resource and stats rendering is clean.
renderResourcesandrenderStatsuse utility functions (renderNumber,renderTroops,translateText) for consistent formatting. Trading status uses color-coded text (yellow for stopped, green for active).
498-577: Alliance list and expiry rendering look good.
renderAlliancesdisplays a scrollable list with hover states and color-coded names.renderAllianceExpiryguards against null (line 561) and uses color-coded text for urgency.
579-694: Action rendering is modular and correct.The
renderActionsmethod uses theactionButtoncomponent for consistent, permission-driven button rendering. The embargo toggle logic (lines 650-667) correctly maps "stop_trade" tohandleEmbargoClick(emits "start") and "start_trade" tohandleStopEmbargoClick(emits "stop"), which aligns with server semantics.Based on past review comments, the embargo toggle inversion has been resolved.
696-799: Main render is clean and accessible.The render method includes:
- Guards against missing player/tile/owner (lines 699-708).
- Backdrop click-to-close semantics (line 744) with propagation stop inside the panel (line 749).
- Conditional traitor styling (line 753) with inline animation (lines 712-737).
- Accessible close button (lines 756-764) with aria-label and title.
- Modular rendering of identity, resources, stats, alliances, expiry, and actions.
Description:
Fixes #2015
Improved the Player Panel UI for better usability and appearance.
Screenshots
Future Plan
Add a modal for sending gold and troops to other players from the Player Panel.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
abodcraft1