refactor: DRY consolidation, health de-sync fix, component decomposition

[SYMBaiEX]

Summary

Depends on: #1145 (PR1)

DRY Consolidation

• Export calculateCombatLevel, normalizeCombatSkills, getXPForLevel, getHpPercent, getHpColor, prayer type guards from shared
• Replace 14+ local client duplicates with shared imports (combat level, XP, HP color, prayer)
• Export ContextMenuAction from shared, derive client type via Omit
• Export MAX_COINS from CoinPouchSystem, replace 9+ hardcoded 2147483647 values
• Deduplicate formatGoldValue via existing compactNumber, remove dead toTotalCopper

Bug Fix

• Health de-sync after PvE combat: StatsComponent had TWO health objects (this.health and this.data.health) that were out of sync. damagePlayer() only updated this.data.health, leaving this.health stale. When constitution XP was gained on mob kill, handleLevelUp() read the stale value, causing emitPlayerUpdate() to broadcast max health to the client.
• Fix CombatPanel.tsx || → ?? for health fallback (was showing 100/100 flash)
• Fix CompactStatusHUD.tsx hardcoded 10/10 defaults → 0/1

Component Decomposition

• Decompose 1345-line CombatPanel into focused sub-components (combat/)
• Decompose 1560-line Minimap into MinimapRenderer + useMinimapInteraction hook

Audit Fixes

• XP formula alignment with SkillsSystem/Solidity contract (13,034,394 at level 99)
• Combat level formula in PlayerSystem consolidated to use shared calculateCombatLevel
• imageCache error TTL (30s retry after transient CDN failures)
• React.memo on CombatStyleSelector, CombatBonusesDisplay, AutoRetaliateToggle

Test plan

• bun run build:shared succeeds
• npx tsc --noEmit — 0 new errors across all packages
• Fight and kill a goblin — health stays at post-combat value, no snap to max
• Goblin respawn — health unchanged
• CombatPanel never flashes 100/100
• All minimap features work (terrain, pips, roads)

[github-actions]

PR Review: DRY Consolidation, Health De-sync Fix, Component Decomposition

Thorough PR that addresses real duplication, fixes a legitimate bug, and improves code organization. Here's my analysis:

---

Health De-sync Fix — Well Diagnosed

The root cause analysis in PlayerSystem.ts is solid: StatsComponent had two health objects (this.health and this.data.health) that diverged when damagePlayer() only updated one. The fix correctly syncs both in damagePlayer() and handleSkillsUpdate().

One concern in handleSkillsUpdate():

const healthMaxChanged = constitutionLevel !== player.health.max;
if (healthMaxChanged) {
  player.health.max = constitutionLevel;
  if (player.health.current > player.health.max) {
    player.health.current = player.health.max;
  }
}

This implicitly couples constitution level to health max (1 level = 1 HP). If the formula ever changes (e.g., constitution level × 10 like OSRS Hitpoints), this will silently break. The comment says "same formula as initial registration at line 693" — consider extracting a shared helper like constitutionToMaxHp(level) so both call sites stay in sync, or at minimum add a cross-reference comment at the registration site pointing back here.

---

DRY Consolidation — Clean Execution

• 14 duplicate implementations replaced with shared imports (calculateCombatLevel, getHpPercent, getHpColor, prayer type guards, XP functions). This is exactly the right approach.
• MAX_COINS exported from CoinPouchSystem: Good single source of truth. The 9 hardcoded 2147483647 values are now traceable.
• formatGoldValue → compactNumber delegation: Clean dedup, removing the 18-line reimplementation.
• toTotalCopper() removal: Confirmed unused — clean removal with export list update.

Minor observation on normalizeCombatSkills call sites: The same 7-field object literal (attack: skills.attack?.level, strength: skills.strength?.level, ...) is repeated verbatim in 6 places across AgentSkillsPanel, AgentSummaryCard (×2), CombatPanel, SkillsPanel, and StatsPanel. Consider a thin wrapper like combatSkillsFromStats(skills) in the client to DRY this further, or accepting Partial<Skills> directly in calculateCombatLevel.

---

XPCalculator.ts — Correct but Quadratic

The accumulative formula produces the correct values (13,034,394 at level 99, matching SkillsSystem and the Solidity contract). However:

• getLevelForXP() scans from level 99 down, calling getXPForLevel() for each level — and getXPForLevel() itself is O(L). This makes getLevelForXP() O(L²) in the worst case.
• The old client implementation used a pre-computed XP_TABLE array with O(1) lookup and binary search for getLevelForXP.
• Since these are client-only UI utilities (as the doc comment states), the performance difference is negligible for single calls. But if ever called in a hot loop (e.g., rendering a list of skills), the old approach was strictly better.

Not a blocker — just flagging the regression in algorithmic complexity versus the removed code.

---

Component Decomposition — Well Structured

• CombatPanel (1345→448 lines): Clean split into CombatStyleSelector, CombatBonusesDisplay, AutoRetaliateToggle, SpecialAttackBar, and StyleIcons. The orchestrator pattern is the right call — state management stays in the parent, sub-components are pure-ish renderers.
• Minimap (1560→667 lines): MinimapRenderer.ts (pure canvas 2D functions, zero React deps) and useMinimapInteraction.ts (all user interaction) are clean separations. The renderer being React-free is particularly good — it's testable in isolation.
• React.memo on CombatStyleSelector, CombatBonusesDisplay, AutoRetaliateToggle: Appropriate — these receive stable props from the parent and re-render unnecessarily during combat state churn.

One note on drawRoadsAndBuildings: The function now accepts projectedRoads: ProjectedRoad[] as an input parameter and mutates it in place (projectedRoads.length = 0; ... projectedRoads.push(...)). This mutation-through-parameter pattern works but is a subtle contract — callers must know the array is cleared and reused. Consider documenting this or making it explicit (e.g., the function returns the array instead).

---

Bug Fixes — Good Catches

• CombatPanel.tsx || → ??: Correct. || { current: 100, max: 100 } would trigger on 0 (falsy), showing a flash of 100/100 when health is actually 0. ?? only triggers on null/undefined.
• CompactStatusHUD.tsx 10/10 → 0/1: Better default — showing 10/10 before data arrives looks like a real value; 0/1 clearly signals "no data yet" and avoids division-by-zero in percentage calculations.
• imageCache.ts error TTL: Smart addition. A 30-second retry window prevents permanent image blackholes from transient CDN issues without creating retry storms.

---

useMinimapInteraction — Improvement in Network Safety

// Old: hard cast + direct call
(world as unknown as WorldNetworkSend).network.send("moveRequest", { ... });

// New: optional chaining
world.network?.send?.("moveRequest", { ... });

This is strictly safer — the old code would throw if network was null. Also importing TerrainSystem directly instead of maintaining a local TerrainSystemLike stub is the right call.

---

Minor Nits

1. DuelHUD.tsx hpColorToTheme mapping: The mapping from OSRS hex colors (#22c55e, #eab308, #ef4444) to theme semantic colors is fragile — if getHpColor() return values ever change, this silently falls through to theme.colors.state.danger. Consider using an enum/constant instead of raw hex strings as keys.

2. useModalPanels.ts isDuelCompletedPayload: The type guard only checks typeof obj.won === "boolean". This is very permissive — any object with a boolean won field would pass. Since this is a narrowing guard on event data from the server, consider checking at least one more field (e.g., opponentName or the object having fewer unexpected fields).

3. _collapsible unused parameter: In useMinimapInteraction, the destructured collapsible: _collapsible is bound but never referenced. This suggests the hook doesn't actually need it — consider removing it from the options interface.

---

Summary

This is a high-quality consolidation PR. The health de-sync fix is well-analyzed and correctly implemented. The DRY work removes genuine duplication without over-abstracting. The component decomposition follows clean patterns. The normalizeCombatSkills call-site repetition and XPCalculator algorithmic complexity are the main things I'd consider addressing, but neither is a blocker.

[Copilot]

Pull request overview

This PR consolidates previously duplicated combat/XP/HP/currency utilities into @hyperscape/shared, fixes a server-side health desync edge case after constitution level-ups, and decomposes large UI components (CombatPanel/Minimap) into smaller, testable units.

Changes:

• Add shared OSRS-style XP ↔ level utilities, HP bar utilities, combat-level helpers, prayer payload guards, and shared currency cap constants.
• Fix health max/current synchronization across PlayerSystem/StatsComponent to prevent post-combat health snapping to max after constitution XP gain.
• Refactor CombatPanel into subcomponents and Minimap into a renderer + interaction hook; improve image-cache error retry behavior.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/shared/src/utils/game/XPCalculator.ts	New shared XP/level conversion utilities for UI use.
packages/shared/src/utils/game/index.ts	Re-export XP calculator from shared game utils barrel.
packages/shared/src/utils/game/CombatUtils.ts	Add shared HP percent + HP color utilities.
packages/shared/src/types/game/prayer-types.ts	Add exported runtime type guards for prayer payloads.
packages/shared/src/types/bank-operations.ts	Replace hardcoded max quantity with shared MAX_COINS.
packages/shared/src/systems/shared/character/PlayerSystem.ts	Sync health current/max with StatsComponent + constitution-derived max health.
packages/shared/src/systems/shared/character/InventorySystem.ts	Replace hardcoded max stack quantity with MAX_COINS.
packages/shared/src/systems/shared/character/CoinPouchSystem.ts	Export DEFAULT_STARTING_COINS and MAX_COINS for reuse.
packages/shared/src/index.ts	Export new shared utilities/constants/types from the main shared entrypoint.
packages/shared/src/index.client.ts	Export new shared utilities/constants/types from the client shared entrypoint.
packages/shared/src/extras/ui/imageCache.ts	Add error TTL to retry transient CDN image failures.
packages/shared/src/data/NoteGenerator.ts	Use MAX_COINS for noted item max stack size.
packages/shared/src/constants/interaction.ts	Use MAX_COINS for input MAX_QUANTITY.
packages/shared/src/constants/BankingConstants.ts	Use MAX_COINS for MAX_ITEM_STACK.
packages/server/src/systems/ServerNetwork/duel-settlement.ts	Use MAX_COINS for overflow checks during stake transfers.
packages/client/src/ui/components/StatusBar.tsx	Swap local HP color logic for shared getHpColor.
packages/client/src/screens/AgentMonitorScreen.tsx	Replace local XP table + HP color logic with shared utilities.
packages/client/src/hooks/useModalPanels.ts	Add payload type guard for duel completion modal handling.
packages/client/src/game/systems/currency/index.ts	Stop exporting removed dead helper toTotalCopper.
packages/client/src/game/systems/currency/currencyUtils.ts	Use MAX_COINS in currency definitions; dedupe formatGoldValue via compactNumber.
packages/client/src/game/panels/StatsPanel.tsx	Use shared combat-level helpers instead of local formula.
packages/client/src/game/panels/SkillsPanel.tsx	Use shared combat-level + XP threshold helpers.
packages/client/src/game/panels/PrayerPanel.tsx	Use shared prayer payload guards instead of local duplicates.
packages/client/src/game/panels/DuelPanel/DuelHUD.tsx	Use shared HP percent/color helpers; map to theme colors.
packages/client/src/game/panels/CombatPanel.tsx	Orchestrator refactor to compose new combat subcomponents and shared combat-level helpers.
packages/client/src/game/panels/combat/types.ts	New shared prop/event/type definitions for combat panel decomposition.
packages/client/src/game/panels/combat/typeGuards.ts	New runtime event guards extracted from CombatPanel.
packages/client/src/game/panels/combat/StyleIcons.tsx	Extract combat SVG icon components + constants.
packages/client/src/game/panels/combat/SpecialAttackBar.tsx	New special attack bar component (future-facing).
packages/client/src/game/panels/combat/index.ts	Barrel exports for combat panel subcomponents/types/guards.
packages/client/src/game/panels/combat/CombatStyleSelector.tsx	New combat style banner grid component (drag + cooldown UI).
packages/client/src/game/panels/combat/CombatBonusesDisplay.tsx	New HP/level/target HP + stat row component.
packages/client/src/game/panels/combat/AutoRetaliateToggle.tsx	New auto-retaliate toggle component extracted from CombatPanel.
packages/client/src/game/interface/CompactStatusHUD.tsx	Fix HP defaults to avoid incorrect 10/10 display.
packages/client/src/game/hud/xp-orb/useXPOrbState.ts	Replace local XP table/search with shared getXPForLevel/getLevelForXP.
packages/client/src/game/hud/useMinimapTerrainCache.ts	Type the terrain system as TerrainSystem and use generic getSystem().
packages/client/src/game/hud/useMinimapInteraction.ts	New hook extracting minimap zoom/resize/click-to-move/collapse behavior.
packages/client/src/game/hud/StatusBars.tsx	Use shared getHpPercent for HP bar percent calculation.
packages/client/src/game/hud/MinimapRenderer.ts	New pure drawing module for minimap rendering + icon cache helpers.
packages/client/src/game/hud/Minimap.tsx	Minimap orchestrator refactor to use renderer + interaction hook.
packages/client/src/game/hud/EntityContextMenu.tsx	Use shared ContextMenuAction type and derive client type via Omit.
packages/client/src/game/dashboard/AgentSummaryCard.tsx	Use shared combat-level helpers instead of local formula.
packages/client/src/game/dashboard/AgentSkillsPanel.tsx	Use shared combat-level + XP utilities; align skills typing with shared Skills.

Comments suppressed due to low confidence (1)

packages/client/src/hooks/useModalPanels.ts:817

• duelCompleted payload coming from ClientNetwork uses a winner: boolean field, but this handler/type guard expects won. With the new guard in place, valid duelCompleted events will be ignored and the duel result modal will never show. Align the payload type/guard and mapping to use winner (or accept both and normalize).

    const rawData: unknown = payload.data;
    if (!isDuelCompletedPayload(rawData)) return;
    const completedData = rawData;
    setDuelData(null);
    setDuelResultData({
      visible: true,
      won: completedData.won ?? false,

---

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

[github-actions]

PR Review: DRY consolidation, health de-sync fix, component decomposition

Overall this is a high-quality PR — the DRY consolidation is well-motivated, the health de-sync fix addresses a real root cause, and the component decompositions are cleanly done. A few items worth addressing before merge:

---

Bug: HP color threshold mismatch in AgentMonitorScreen.tsx

The old local hpColor() used 60%/30% thresholds (ratio > 0.6 / > 0.3), while the new shared getHpColor() uses 50%/25% thresholds. The hpClass() function now delegates to getHpColor(ratio * 100), which means the CSS class boundaries have silently shifted:

// OLD: "high" when ratio > 0.6 (60%), "mid" when ratio > 0.3 (30%)
// NEW: "high" when ratio > 0.5 (50%), "mid" when ratio > 0.25 (25%)

A player at 55% HP now gets "high" (green) instead of "mid" (yellow). This could be intentional (aligning with the OSRS standard), but if the old thresholds were deliberate for the agent monitor dashboard, it's a regression. Please confirm this is intentional.

Code smell: hex-string-to-theme mapping in DuelHUD.tsx

const hpColorToTheme: Record<string, string> = {
  "#22c55e": theme.colors.state.success,
  "#eab308": theme.colors.state.warning,
  "#ef4444": theme.colors.state.danger,
};
const getHealthColor = (percent: number): string =>
  hpColorToTheme[getHpColor(percent)] ?? theme.colors.state.danger;

This creates a fragile coupling to the exact hex values returned by getHpColor(). If anyone changes the hex constants in CombatUtils.ts, this silently falls through to danger for all health levels. The same pattern appears in AgentMonitorScreen.tsx (hpClass comparing color === "#22c55e").

Suggestion: Either (a) export a companion getHpTier(percent): "high" | "mid" | "low" from shared that consumers can map to their own colors, or (b) at minimum add a comment in getHpColor warning that return values are depended upon as keys.

Missing useCallback on handlers passed to memoized children

CombatPanel.tsx wraps CombatStyleSelector, CombatBonusesDisplay, and AutoRetaliateToggle in React.memo, but changeStyle and toggleAutoRetaliate are plain arrow functions that are recreated every render:

const changeStyle = (next: string) => { ... };
const toggleAutoRetaliate = () => { ... };

These are passed as onStyleChange and onToggle — which defeats the React.memo optimization on those children. Wrapping both in useCallback (with appropriate deps) would make the memos effective.

formatGoldValue behavior change

The old implementation produced values like 1.5K (floor to one decimal), while compactNumber may produce different formatting. For example, old code: Math.floor((value % 1000) / 100) gives one fractional digit, while compactNumber may round differently. This matters for trade/duel panels where gold display precision affects player trust. Worth verifying the output format matches for values like 1,500 / 15,500 / 1,500,000.

Health de-sync fix — well done, one observation

The dual-sync approach in PlayerSystem.emitPlayerUpdate() and handleSkillsUpdate() is the right fix. The root cause analysis in the commit message is excellent.

One thing to watch: handleSkillsUpdate() now caps health.current to health.max when constitution levels up:

if (player.health.current > player.health.max) {
  player.health.current = player.health.max;
}

This prevents HP from exceeding max, but OSRS actually heals to new max on constitution level-up. If that's desired behavior, this should set current = max instead of capping. If not, the current code is correct — just flagging the design choice.

XPCalculator formula — minor precision note

const xp = Math.floor(l - 1 + 300 * Math.pow(2, (l - 1) / 7)) / 4;
cumulative = Math.floor(cumulative + xp);

The Math.floor only wraps the numerator (l - 1 + 300 * ...), then divides by 4, and the outer Math.floor truncates. This matches the OSRS wiki formula. The claim of 13,034,394 at level 99 should be verified with a unit test to catch any future drift.

Positive notes

• MAX_COINS consolidation is clean — single source of truth, no new files, just export const
• imageCache error TTL is a solid improvement — 30s retry for transient CDN failures instead of permanent blocking
• Prayer type guards are well-structured with proper runtime checks
• Minimap decomposition is well-factored — MinimapRenderer as pure canvas functions with zero React dependencies is the right pattern
• CombatPanel decomposition reduces a 1345-line file to 448 lines with logical sub-components
• Component exports via index.ts barrel file keeps imports clean
• isDuelCompletedPayload type guard replacing unsafe as unknown as cast is a good safety improvement