refactor: client cleanup, dead code removal, shared exports & Yoga UI by SYMBaiEX · Pull Request #1145 · PlayHyperia/hyperforge

SYMBaiEX · 2026-04-10T18:58:40Z

Summary

Remove ~2,500 lines of dead code (WebSocketManager, Fields, CurvePane, CurvePreview, useQuestLog, ItemData, HintContextType, orphaned ui.ts)
Consolidate duplicate types into game/types/
Consolidate formatGoldValue into currency utils
Move SettingsPanel into directory structure
Tag internal exports, clean deprecated aliases
Export accessibility, complexity, and combat-level utils from shared
Fix client-specific declaration file generation
Add UIImage node for Yoga UI system (image cache, node, scroll support)

Test plan

npx tsc --noEmit -p packages/shared/tsconfig.json — 0 errors
npx tsc --noEmit -p packages/client/tsconfig.json — 0 new errors
npm run lint — no new errors
Game loads and all panels render correctly
No missing imports or broken references

Delete websocket-manager.ts (~435 LOC) and its barrel export from lib/index.ts. This module was never imported by any consumer — the CharacterSelectScreen uses its own local connection state, and the game client uses Hyperscape's internal world.init() networking.

- Delete types/player.ts — its types were duplicated in game/types/ - Add quantity field to RawEquipmentSlot in game/types/equipment.ts - Create game/types/events.ts as canonical NetworkEvents definition - Update useModalPanels to import from game/types instead of local copy - Re-route types/index.ts barrel to re-export from game/types/ Eliminates type duplication between types/player.ts and game/types/, establishing game/types/ as the single source of truth for client UI type definitions.

Move OSRS-style gold formatting to the canonical location in game/systems/currency/currencyUtils.ts. Remove duplicate definitions from DuelPanel/utils.ts and TradePanel/utils.ts. Update all consumers (StakesScreen, ConfirmScreen, DuelResultModal, StakeGrid, TradePanel) to import from the central currency system.

Resolve naming conflict where SettingsPanel.tsx (1,547 LOC) sat as a sibling file next to the SettingsPanel/ directory containing its sub-components. Move the file into the directory and update index.ts to export from the local file instead of the parent. Fix relative import paths that now need an additional level of traversal.

Remove unused ItemData interface from game/systems/types.ts — it was never imported by any consumer. The client correctly uses getItem() from @hyperscape/shared (152 usages across 58 files) to read from the ITEMS Map populated by DataManager from server manifests. No client-side item definitions found that bypass the manifest system.

Delete useQuestLog.ts (~440 LOC) — it was never imported by any consumer. QuestsPanel manages quest data directly from server events, and the quest selection store (questStore.ts) handles cross-panel quest state. The quest status sync (useQuestStatusSync) correctly bridges server events into the Zustand store for minimap quest pips.

Delete ~950 LOC of unreferenced field editor components. Fields.tsx defined 10 React field components never imported by any consumer. CurvePane.tsx and CurvePreview.tsx were only imported by Fields.tsx.

The exported HintContextType had zero importers. The production HintContext lives in ui/components/Hint.tsx with its own local type.

- Add @internal JSDoc to UI/UIView/UIText node exports and Yoga type exports in shared/index.client.ts — these are engine-internal 3D UI nodes, not for client React/CSS usage - Remove deprecated LRUCache/createMonitoredCache/cacheRegistry barrel aliases from client lib/index.ts (never imported by any consumer)

Remove 389-line file of React prop interfaces (ContextMenuAction, FieldTextProps, WindowProps, etc.) that were never exported from the package or imported by any consumer. The production ContextMenuAction lives in systems/client/interaction/types.ts with the correct shape (handler + priority + styledLabel).

Wire up orphaned shared types that were built for client consumption but never exported from the package. Includes COLORBLIND_PALETTES, COMPLEXITY_MODE_CONFIGS, getCombatLevelColor, and related types.

The ./client export in package.json pointed to framework.d.ts (full server+client types) instead of a client-specific declaration. This meant new exports added to index.client.ts were invisible to client consumers. Fix: build.mjs now copies index.client.d.ts to framework.client.d.ts, and package.json ./client export points to it. Client consumers now see only the types actually exported for client use.

Replace duplicate type definitions in client with re-exports from @hyperscape/shared. Both accessibility.ts (93 LOC) and complexity.ts (127 LOC) were byte-for-byte identical between packages. - Add accessibility and complexity exports to shared/index.ts - Add ComplexityFeatures to shared export lists - Client files now re-export from @hyperscape/shared as single source of truth for COLORBLIND_PALETTES, COMPLEXITY_MODE_CONFIGS, etc.

Create imageCache.ts — module-level singleton that loads and caches HTMLImageElement instances for use with Canvas 2D drawImage(). Used by the upcoming UIImage node to render item icons and game assets. Features: coalesced callbacks per URL, 256-entry LRU eviction, server-safe (typeof window guard), crossOrigin anonymous for CDN.

Create UIImage.ts — renders images in canvas-based UI panels with full Yoga flexbox layout support. Follows UIView/UIText patterns. Features: - Async image loading via imageCache with automatic redraw on load - objectFit modes: fill, contain, cover, none, scale-down - borderRadius clipping via imageRoundRect - Yoga measure function for intrinsic sizing from natural dimensions - All standard layout props: margin, padding, absolute positioning, flexBasis/Grow/Shrink, display toggle The UIImageData type was already defined in types/rendering/nodes.ts — this commit provides the implementation.

Add UIImage + uiimage aliases to nodes/index.ts for factory support (createNode("uiimage", data)). Export from both index.ts and index.client.ts entry points. Rebuild generates declarations in framework.d.ts and framework.client.d.ts.

Extend UIView with overflow clipping and scroll offset properties: - overflow: "visible" | "hidden" | "scroll" (default "visible") - scrollX/scrollY: pixel offsets for scrolled content - Yoga OVERFLOW_HIDDEN prevents parent expansion for clipped content - Canvas ctx.save()/clip()/restore() clips children to view bounds - Scroll offsets subtract from child draw coordinates - roundRect clip path respects borderRadius for rounded scroll areas Enables scrollable lists, inventory grids, and chat panels in the Yoga UI node system.

Create attachScrollHandler() — wires wheel events on a root UI node to a scrollable UIView child. Handles pointer-in-bounds detection, content height calculation from Yoga children, scroll clamping, speed scaling, and optional shift+wheel horizontal scroll. Returns a cleanup function for teardown.

DuelArenaVisualsSystem: import LOBBY_CENTER_X/Z, LOBBY_WIDTH/LENGTH and HOSPITAL_CENTER_X/Z, HOSPITAL_WIDTH/LENGTH from data/arena-layout.ts (constants already existed, just missing import after armor pipeline merge) EquipmentVisualSystem: import getItem from data/items.ts (function already existed, missing import after armor pipeline merge)

github-actions · 2026-04-10T19:02:22Z

PR Review: refactor: client cleanup, dead code removal, shared exports & Yoga UI

Overall this is a solid cleanup PR — removing ~2,500 lines of dead code with no broken imports is a win. The new UIImage node, image cache, and scroll handler are well-structured. A few issues worth addressing before merge:

Bug: `UIView.copy()` missing new properties

packages/shared/src/nodes/UIView.ts — the copy() method (line 328) copies all existing _ properties but omits the three newly added ones: _overflow, _scrollX, _scrollY. This means cloning a UIView will silently lose overflow/scroll state.

Fix:

copy(source: UIView, recursive: boolean) {
    super.copy(source, recursive);
    // ... existing copies ...
    this._flexShrink = source._flexShrink;
+   this._overflow = source._overflow;
+   this._scrollX = source._scrollX;
+   this._scrollY = source._scrollY;
    this._flexWrap = source._flexWrap;
    // ...

Bug: `imageCache.ts` LRU eviction can leave cache oversized

In evictIfNeeded() (line 42), when a loading entry is encountered, it's rotated to the back and the loop breaks. If many loading entries accumulate at the front, the cache can exceed MAX_CACHE_SIZE because it only tries once. Consider using continue instead of break, or limiting the rotation attempts to avoid an infinite loop on a cache full of loading entries:

function evictIfNeeded(): void {
+  let attempts = 0;
  while (insertionOrder.length > MAX_CACHE_SIZE) {
    const oldestUrl = insertionOrder[0];
    const entry = cache.get(oldestUrl);
    if (entry && entry.status !== "loading") {
      cache.delete(oldestUrl);
      insertionOrder.shift();
    } else {
      insertionOrder.shift();
      if (oldestUrl) insertionOrder.push(oldestUrl);
-     break;
+     if (++attempts >= insertionOrder.length) break; // all loading, give up
    }
  }
}

Type consistency: `UIViewData.overflow` vs `OverflowMode`

In packages/shared/src/types/rendering/nodes.ts, UIViewData.overflow is typed as inline "visible" | "hidden" | "scroll" (line 707), but OverflowMode is defined as the exact same union on line 712. Consider using the named type for consistency:

overflow?: OverflowMode;

Unsafe casts in `scrollHandler.ts`

packages/shared/src/extras/ui/scrollHandler.ts has several as unknown as { ... } casts to access internal properties:

Line 86: (rootUI as unknown as { _res: number })._res — accessing a private property via cast
Lines 104–112: Overwrites _onWheel / onWheel via cast to chain event handlers

This is fragile and will silently break if those internal properties are renamed. Consider either:

Exposing a public resolution getter on the UI class
Using a proper event listener pattern (e.g., rootUI.addEventListener('wheel', handler) if available)

Also, the magic numbers for Yoga edge constants (0, 1, 2, 3 on lines 78–83) should use named constants from the Yoga module for readability.

imageCache: error entries never expire

Once an image fails to load, imageCache.ts caches it as { status: "error" } permanently — no retry, no TTL. If a CDN blip causes a transient failure, that icon stays broken for the session. Consider either:

Adding a timestamp to error entries and retrying after a cooldown
Not caching errors at all (simple, and redundant loads are cheap)

Nits (non-blocking)

UIImageData.objectFit (nodes.ts:725) — the type is "fill" | "contain" | "cover" | "none" | "scale-down" | string, but | string makes the literal union meaningless (any string matches). If you want to allow arbitrary values, use string; if you want to restrict it, drop the | string.
@internal annotations on index.client.ts exports (lines 267–268) — nice documentation touch. Consider adding the same annotation pattern to the new accessibility/complexity/combat-level exports at the bottom of both index.ts and index.client.ts for consistency.

What looks good

Dead code removal is clean — no dangling imports from any of the 7 deleted files
formatGoldValue consolidation into currencyUtils.ts is correct; all consumers updated
UIImage implementation is thorough: proper Yoga measure function with aspect ratio, object-fit modes matching CSS spec, clean lifecycle management
Discriminated union types in imageCache (LoadingEntry | LoadedEntry | ErrorEntry) are idiomatic TypeScript
Build script change to copy index.client.d.ts → framework.client.d.ts is a straightforward fix for client declaration generation
The new events.ts constants with as const are clean and type-safe

github-actions · 2026-04-10T19:04:38Z

PR Review: refactor: client cleanup, dead code removal, shared exports & Yoga UI

Overall Assessment

Solid cleanup PR that removes ~2,500 lines of dead code and consolidates duplicate types/utilities. The net -1,629 lines is a clear win. The new UIImage node and UIView scroll support are well-structured additions to the engine's Yoga-based UI system.

Positive

Dead code removal is clean: WebSocketManager, Fields.tsx, CurvePane, CurvePreview, useQuestLog, player.ts, types/rendering/ui.ts — all confirmed to have zero remaining consumers. Good diligence.
formatGoldValue consolidation into currencyUtils.ts is the right call — eliminates two divergent implementations.
SettingsPanel directory migration completed properly: the old re-export indirection (export from "../SettingsPanel") is replaced with a direct import from the co-located file.
Accessibility/complexity types promoted to @hyperscape/shared so client re-exports from a single source of truth — correct direction.
UIImage node follows existing UIView/UIText patterns closely (Yoga layout, proxy pattern, mount/unmount lifecycle). The computeObjectFit function is a clean implementation of CSS object-fit semantics.
Client-specific .d.ts generation fix (framework.client.d.ts) addresses a real gap in the build pipeline.

Issues & Suggestions

1. `formatGoldValue` — Subtle behavioral change for DuelPanel consumers (minor)

The old DuelPanel implementation used toFixed(1) for all tiers including billions (e.g., "1.5B"). The consolidated version uses Math.floor for billions (e.g., "1B" — no decimal). For values like 1,500,000,000, the old code returned "1.5B" while the new code returns "1B". Consider whether the DuelPanel's billion-tier display should retain the decimal:

// Current (line 376-377 of currencyUtils.ts)
const b = Math.floor(value / 1000000000);
return `${b}B`;

// Consider:
const b = Math.floor(value / 1000000000);
const remainder = Math.floor((value % 1000000000) / 100000000);
return remainder > 0 ? `${b}.${remainder}B` : `${b}B`;

2. `UIImage` — `this.ui!` non-null assertions in setters (medium)

In UIImage.ts, the draw() method correctly guards with if (!this.ui) return, but several property setters (e.g., width, height, margin) use this.ui!._res without checking if ui is set. Before mount() is called, ui is undefined. If a user sets width before mounting, this throws.

The yogaNode optional chaining (this.yogaNode?.setWidth(...)) already handles the pre-mount case for the Yoga side, but the _res scaling will crash. Consider guarding:

// In width setter, etc:
this.yogaNode?.setWidth(
  this._width === null || !this.ui ? undefined : this._width * this.ui._res,
);

Or just skip the Yoga update when ui is not yet set (mount will handle it).

3. `scrollHandler.ts` — Fragile internal API access (minor)

const res = (rootUI as unknown as { _res: number })._res ?? 2;

and

const prevHandler = (rootUI as unknown as { _onWheel?: (e: UIWheelEvent) => void })._onWheel;

These cast through unknown to access private members. If UI._res or the wheel handler mechanism changes, this will silently break. Consider adding a public accessor to the UI class (e.g., get resolution()) or at minimum a comment documenting which internal API this depends on.

4. `imageCache.ts` — Eviction can stall on loading entries (minor)

The eviction loop breaks when it encounters a loading entry at the front, rotating it to the back. If many concurrent loads are in-flight, the cache can grow significantly above MAX_CACHE_SIZE since only one entry gets rotated per evictIfNeeded() call. Consider using continue instead of break to evict all eligible non-loading entries in a single pass:

function evictIfNeeded(): void {
  let attempts = insertionOrder.length;
  while (insertionOrder.length > MAX_CACHE_SIZE && attempts-- > 0) {
    const oldestUrl = insertionOrder.shift()!;
    const entry = cache.get(oldestUrl);
    if (entry && entry.status === "loading") {
      insertionOrder.push(oldestUrl); // rotate loading entry to back
    } else {
      cache.delete(oldestUrl);
    }
  }
}

5. `DuelArenaVisualsSystem.ts` & `EquipmentVisualSystem.ts` — Unrelated import additions (nit)

These two files add imports (arena-layout constants, getItem) that appear to fix pre-existing missing imports rather than being part of this cleanup PR. Consider whether these belong in a separate commit for cleaner git history, or at minimum call them out in the PR description.

6. No tests for new UIImage/scroll functionality

The PR adds a new UIImage node class (~700 lines) and scroll behavior to UIView. Per the project's testing philosophy (real Hyperscape instances, no mocks), it would be good to have at least basic visual regression tests for these new rendering paths. Even a simple test that creates a UIImage, sets a src, and verifies it renders after the image loads would catch regressions.

Security

No secrets or credentials introduced.
img.crossOrigin = "anonymous" in the image cache is correct for CDN assets.
imageCache accepts pre-resolved HTTP URLs only (per the docstring) — no asset:// protocol handling, which is good.

Performance

Image cache with LRU eviction at 256 entries is reasonable for UI icons.
The loadCachedImage callback coalescing pattern avoids redundant loads.
scrollHandler recalculates content height by iterating all Yoga children on every wheel event — fine for typical UI trees but could be cached if scroll containers get large.

Summary

This is a well-executed cleanup PR. The dead code removal is safe (no remaining consumers), the type consolidation reduces duplication, and the new UIImage/scroll features follow established engine patterns. The main items to address are the formatGoldValue billion-tier regression and the this.ui! non-null assertions in UIImage setters. The rest are minor improvements.

Copilot

Pull request overview

This PR performs a broad client/shared cleanup and refactor, consolidates duplicated UI/game types and exports, fixes shared client declaration generation, and extends the Yoga/canvas UI system with image rendering and initial scroll/overflow support.

Changes:

Remove dead/duplicated client and shared code, and centralize canonical game/UI types.
Add Yoga UI enhancements: UIImage node, shared image cache, and UIView overflow/scroll plumbing (+ scroll helper).
Fix @hyperscape/shared client types export to use a client-specific .d.ts output.

Reviewed changes

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

Show a summary per file

File	Description
packages/shared/src/types/rendering/ui.ts	Removed legacy UI-related shared type bundle (dead/duplicated).
packages/shared/src/types/rendering/nodes.ts	Adds `UIViewData` overflow/scroll fields and exports `OverflowMode`.
packages/shared/src/nodes/UIView.ts	Adds overflow clipping + scroll offsets and new proxy/getters/setters.
packages/shared/src/nodes/UIImage.ts	Introduces a new Yoga/canvas-backed UI image node with object-fit and async loading.
packages/shared/src/nodes/index.ts	Exports `UIImage` and lowercase alias `uiimage` for `createNode()`.
packages/shared/src/index.ts	Re-exports `UIImage` and exposes accessibility/complexity/combat-level utilities from shared.
packages/shared/src/index.client.ts	Re-exports `UIImage`, UI node types, and additional shared utilities; adds internal-notes comments.
packages/shared/src/extras/ui/scrollHandler.ts	Adds helper to attach wheel-driven scrolling behavior for `UIView` overflow scrolling.
packages/shared/src/extras/ui/index.ts	Re-exports new UI helpers (`imageCache`, `scrollHandler`).
packages/shared/src/extras/ui/imageCache.ts	Adds shared image loading/cache layer for canvas UI rendering.
packages/shared/scripts/build.mjs	Copies `index.client.d.ts` → `framework.client.d.ts` during declaration generation.
packages/shared/package.json	Updates `exports["./client"].types` to `./build/framework.client.d.ts`.
packages/client/src/ui/types/complexity.ts	Switches to re-export complexity types/constants from `@hyperscape/shared`.
packages/client/src/ui/types/accessibility.ts	Switches to re-export accessibility types/constants from `@hyperscape/shared`.
packages/client/src/ui/components/index.ts	Removes stale comment re: Fields relocation.
packages/client/src/types/ui.ts	Removes `HintContextType` from exported client types.
packages/client/src/types/player.ts	Removes legacy player state types/constants (moved to game/types).
packages/client/src/types/index.ts	Re-points player state exports to `game/types` and removes `HintContextType`.
packages/client/src/lib/websocket-manager.ts	Removes deprecated/unreferenced WebSocket manager implementation.
packages/client/src/lib/index.ts	Stops exporting the removed WebSocket manager and deprecated cache aliases.
packages/client/src/hooks/useModalPanels.ts	Uses canonical `NetworkEvents` from `game/types` instead of a local copy.
packages/client/src/game/types/index.ts	Adds canonical exports for `NetworkEvents` + `NetworkEventName`.
packages/client/src/game/types/events.ts	Introduces canonical network event constants + union type.
packages/client/src/game/types/equipment.ts	Restores `quantity?: number` on `RawEquipmentSlot` in canonical equipment types.
packages/client/src/game/systems/types.ts	Removes unused `ItemData` interface from game systems types.
packages/client/src/game/systems/quest/useQuestLog.ts	Removes unused quest log hook implementation.
packages/client/src/game/systems/quest/index.ts	Removes exports for the deleted `useQuestLog` hook.
packages/client/src/game/systems/currency/index.ts	Re-exports consolidated `formatGoldValue` from currency utils.
packages/client/src/game/systems/currency/currencyUtils.ts	Adds consolidated `formatGoldValue` implementation.
packages/client/src/game/panels/TradePanel/utils.ts	Removes local `formatGoldValue` helper (now centralized).
packages/client/src/game/panels/TradePanel/TradePanel.tsx	Imports `formatGoldValue` from the currency system.
packages/client/src/game/panels/SettingsPanel/SettingsPanel.tsx	Updates imports after moving `SettingsPanel` deeper into the directory structure.
packages/client/src/game/panels/SettingsPanel/index.ts	Updates re-export to the new local `SettingsPanel` path.
packages/client/src/game/panels/DuelPanel/utils.ts	Removes local `formatGoldValue` helper (now centralized).
packages/client/src/game/panels/DuelPanel/StakesScreen.tsx	Imports `formatGoldValue` from the currency system.
packages/client/src/game/panels/DuelPanel/DuelResultModal.tsx	Imports `formatGoldValue` from the currency system.
packages/client/src/game/panels/DuelPanel/ConfirmScreen.tsx	Imports `formatGoldValue` from the currency system.
packages/client/src/game/panels/DuelPanel/components/StakeGrid.tsx	Imports `formatGoldValue` from the currency system (path adjusted).
packages/client/src/game/Fields.tsx	Removes large, unused Fields UI implementation.
packages/client/src/game/CurvePreview.tsx	Removes unused curve preview component.
packages/client/src/game/CurvePane.tsx	Removes unused curve editor pane component.

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

Copilot · 2026-04-10T19:05:48Z

    this.yogaNode.setFlexBasis(this._flexBasis);
    this.yogaNode.setFlexGrow(this._flexGrow);
    this.yogaNode.setFlexShrink(this._flexShrink);
+    if (this._overflow === "hidden" || this._overflow === "scroll") {
+      this.yogaNode.setOverflow(Yoga.OVERFLOW_HIDDEN);
+    }


overflow: "scroll" is currently mapped to Yoga.OVERFLOW_HIDDEN, making it indistinguishable from overflow: "hidden" at the Yoga layout level. If scroll is meant to be a distinct mode, set Yoga overflow to OVERFLOW_SCROLL for the "scroll" case and keep OVERFLOW_HIDDEN for "hidden".

Copilot · 2026-04-10T19:05:49Z

+  set overflow(value: OverflowMode) {
+    if (this._overflow === value) return;
+    this._overflow = value;
+    if (this.yogaNode) {
+      if (value === "hidden" || value === "scroll") {
+        this.yogaNode.setOverflow(Yoga.OVERFLOW_HIDDEN);
+      } else {
+        this.yogaNode.setOverflow(Yoga.OVERFLOW_VISIBLE);
+      }


The overflow setter also maps both "hidden" and "scroll" to Yoga.OVERFLOW_HIDDEN, so switching to overflow="scroll" won’t update Yoga to a scroll overflow mode. Consider setting Yoga.OVERFLOW_SCROLL when value === "scroll", and OVERFLOW_HIDDEN only for value === "hidden".

Copilot · 2026-04-10T19:05:49Z

+    // Sum children's computed heights for total content size
+    let contentHeight = 0;
+    let contentWidth = 0;
+    const childCount = scrollView.yogaNode.getChildCount();
+    for (let i = 0; i < childCount; i++) {
+      const child = scrollView.yogaNode.getChild(i);
+      contentHeight +=
+        child.getComputedHeight() +
+        child.getComputedMargin(1) + // EDGE_TOP
+        child.getComputedMargin(3); // EDGE_BOTTOM
+      contentWidth +=
+        child.getComputedWidth() +
+        child.getComputedMargin(0) + // EDGE_LEFT
+        child.getComputedMargin(2); // EDGE_RIGHT
+    }


contentWidth/contentHeight are computed by summing every child’s width/height, which overestimates the scrollable extent for common layouts (e.g., vertical lists will incorrectly accumulate widths). To clamp scrolling correctly, compute the max extent based on each child’s computed position + size (+ margins), rather than summing dimensions.

Copilot · 2026-04-10T19:05:49Z

+  // Attach to root UI's onWheel
+  const prevHandler = (
+    rootUI as unknown as { _onWheel?: (e: UIWheelEvent) => void }
+  )._onWheel;
+  (rootUI as unknown as { onWheel: (e: UIWheelEvent) => void }).onWheel = (
+    event: UIWheelEvent,
+  ) => {
+    prevHandler?.(event);
+    handler(event);
+  };
+
+  // Return cleanup function
+  return () => {
+    (
+      rootUI as unknown as { onWheel: ((e: UIWheelEvent) => void) | undefined }
+    ).onWheel = prevHandler;
+  };


This chains wheel handlers via a non-existent _onWheel field. UI/Node don’t define _onWheel (or a wheel handler field at all), so prevHandler will always be undefined and cleanup will restore the wrong thing. Use the public onWheel callback (read current rootUI.onWheel, wrap it, then restore it in cleanup), or add a real wheel handler field/API on UI/Node that the input system dispatches to.

Copilot · 2026-04-10T19:05:50Z

+  overflow?: "visible" | "hidden" | "scroll";
+  scrollX?: number;
+  scrollY?: number;
 }

+export type OverflowMode = "visible" | "hidden" | "scroll";
+


UIViewData.overflow repeats the string union instead of reusing the exported OverflowMode type right below. To avoid drift, consider changing the property to overflow?: OverflowMode and keeping the union in one place.

Copilot · 2026-04-10T19:05:50Z

 // Re-export commonly used node classes to satisfy API extractor
+// @internal — These are engine-internal 3D UI nodes using Yoga layout.
+// Client UI uses React/CSS, not these canvas-based nodes.
 export { UI } from "./nodes/UI";
 export { UIView } from "./nodes/UIView";
 export { UIText } from "./nodes/UIText";
+export { UIImage } from "./nodes/UIImage";


If the goal is to have API Extractor treat these exports as internal, // @internal line comments typically won’t be recognized. The repo uses JSDoc /** @internal */ elsewhere (e.g. packages/shared/src/utils/rendering/InstancedMeshManager.ts:2845). Consider switching to a JSDoc @internal tag on the export(s) so tooling can exclude them consistently.

Copilot · 2026-04-10T19:05:50Z

 export type { Physics as PhysicsInterface } from "./types/index";
-// Re-export UI-related types used by UIView/UIText/UI
+// @internal — UI node types for engine-internal 3D UI system (Yoga layout).
+// Client UI uses React/CSS, not these types.
 export type {


Same as above: // @internal comments likely won’t be picked up by API Extractor. Consider using a JSDoc /** @internal */ tag on these type exports if they’re intended to be hidden from the public client API surface.

- UIView.copy(): add missing _overflow, _scrollX, _scrollY copies - UIImage setters: guard this.ui before accessing _res to prevent crash when properties are set before mount() - imageCache evictIfNeeded(): replace break with continue + attempt limit to prevent infinite stall when all entries are loading - UIImageData.objectFit: remove `| string` that made the literal union type meaningless - scrollHandler: replace unsafe cast to private _res with public res getter already exposed on UI class - formatGoldValue: restore toFixed(1) for billion tier to preserve decimal display (e.g. "1.5B" instead of "1B")

github-actions · 2026-04-11T16:14:09Z

Code Review: PR #1145

Overall Assessment

Solid cleanup PR. Removing ~2,500 lines of verified dead code (WebSocketManager, Fields, CurvePane, CurvePreview, useQuestLog, orphaned ui.ts) is valuable, and the new Yoga UI features (UIImage, scroll, overflow) are well-structured. I confirmed with grep that all deleted modules have zero remaining importers. The type consolidation and client declaration fix are both good catches.

A few items below worth addressing before merge:

Bugs / Correctness

1. imageCache.ts — Error callbacks are silently swallowed

When an image fails to load (line 115-118), all coalesced onLoad callbacks are cleared without ever being called. Callers have no way to know the load failed — the callback just never fires. At minimum, consider calling the callbacks so the UI can react (e.g., show a placeholder or retry):

img.addEventListener("error", () => {
  cache.set(url, { status: "error" });
  for (const cb of entry.callbacks) cb(); // Let callers know *something* happened
  entry.callbacks.length = 0;
});

Alternatively, add an onError callback parameter to loadCachedImage().

2. imageCache.ts — Callback exception can leak memory

Line 109-112: If any callback in entry.callbacks throws, subsequent callbacks won't fire and entry.callbacks.length = 0 is never reached. Wrap the loop:

for (const cb of entry.callbacks) {
  try { cb(); } catch (e) { console.error("[imageCache] callback error", e); }
}
entry.callbacks.length = 0;

3. scrollHandler.ts — Unsafe private field access via as unknown

Lines 104-111 use as unknown as { _onWheel ... } to read and write the _onWheel handler. This is fragile — if the UI class renames or restructures how wheel handlers work, this breaks silently at runtime with no type error.

Consider either:

Adding a public onWheel getter/setter to the UI class, or
Using the existing UI public API (if one exists for event subscriptions)

4. UIView.ts — overflow and scrollX/scrollY set directly in constructor, bypassing setters

Lines 136-139: Unlike other properties, overflow, scrollX, and scrollY are assigned via this._overflow = ... directly, skipping the setter. This means the Yoga node's overflow mode isn't configured during construction. When mount() runs, the initial overflow mode is never applied to the Yoga node unless overflow is set again later.

Recommend using this.overflow = ... (the setter) like the other properties do, or explicitly setting the Yoga overflow in mount().

5. UIImage.ts — copy() does shallow copy of _margin array

Line 245: this._margin = source._margin — if margin is a 4-element array, both instances share the same array reference. Mutating one mutates the other. Should be:

this._margin = isArray(source._margin) ? [...source._margin] : source._margin;

Type Safety

6. UIImageData.width/height allows string but implementation expects number | null

In types/rendering/nodes.ts line 718-719:

width?: number | string | null;
height?: number | string | null;

But UIImage.ts constructor casts data.width as number | null | undefined (line 85). If a string is passed, it silently becomes a string that gets multiplied by _res, producing NaN. The type definition should match the implementation — remove | string from both.

7. scrollHandler.ts — Magic number Yoga edge constants

Lines 78-83 use hardcoded numbers (0, 1, 2, 3) for Yoga edge constants instead of importing Yoga.EDGE_TOP, etc. These should use the named constants from the yoga-layout package for readability and resilience against API changes.

Performance

8. imageCache.ts — insertionOrder.shift() is O(n)

Line 48-52: The eviction loop calls shift() on an array, which is O(n) for each call. With MAX_CACHE_SIZE=256 this is fine in practice, but if the cache grows, consider using an index-based approach or a linked list. Not blocking, just a note.

Minor Suggestions (non-blocking)

UIImage.getProxy() (line 557-647): The manual proxy pattern with 15 get/set pairs is verbose but consistent with UIView.getProxy(). Consider adding a comment noting this follows the existing UIView pattern, so future maintainers don't try to "optimize" it differently.
events.ts: Clean constant definition. Consider adding a brief comment per event noting the payload shape, since NetworkEventName only captures the event names, not what data they carry.
currencyUtils.ts line 221: The regex /^(-?[\d.]+)([kKmMbB])?$/ allows multiple decimal points (e.g., "1.2.3k" would match). Consider tightening to /^(-?\d+\.?\d*)([kKmMbB])?$/.
formatGoldValue (line 376): Good that toFixed(1) was restored for the billion tier — the fix commit message calls this out explicitly.

What Looks Good

Dead code removal is well-validated — each commit message explains why the code was unused and what the production replacement is
formatGoldValue consolidation eliminates 3 duplicate definitions cleanly
The client declaration file fix (framework.client.d.ts) is a meaningful build correctness improvement
UIImage object-fit implementation correctly matches CSS spec for all 5 modes
Overflow clipping in UIView.draw() properly uses ctx.save()/ctx.clip()/ctx.restore() with border-radius support
Type re-exports from @hyperscape/shared for accessibility/complexity eliminate byte-for-byte duplicates

Nice work overall. The main items I'd want fixed before merge are #1-5 above (the bugs/correctness section). The rest are improvement suggestions.

- imageCache: fire callbacks on error (callers can show placeholder), wrap callbacks in try/catch to prevent memory leaks - scrollHandler: replace unsafe `as unknown` casts with public UI.onWheel property, use Yoga.EDGE_* constants instead of magic numbers - UIView: use setters in constructor for overflow/scrollX/scrollY so Yoga overflow mode is applied during construction - UIImage.copy(): deep-copy margin array to prevent shared mutation - UIImageData: remove `| string` from width/height (implementation expects number, string would produce NaN)

github-actions · 2026-04-11T16:29:24Z

PR Review: refactor: client cleanup, dead code removal, shared exports & Yoga UI

Overall this is a solid cleanup PR — removing ~2,500 lines of confirmed dead code, consolidating scattered types, and adding useful new Yoga UI primitives. The refactoring commits are well-scoped and the dead code removals are clean (verified zero remaining imports for all deleted files). Below are findings organized by severity.

Bugs

1. Type mismatch between UI.onWheel and scrollHandler (medium severity)

UI.ts:133 declares onWheel with an inline type:

onWheel?: (e: { deltaX: number; deltaY: number; shiftKey: boolean }) => void;

But scrollHandler.ts types the handler parameter as UIWheelEvent, which extends UIPointerEvent and includes coords, deltaZ, deltaMode, etc. The handler accesses event.coords?.x and event.coords?.y for hit-testing — these properties don't exist on the inline type declared on UI. While this works at runtime because JS doesn't enforce types, it means:

The onWheel property on UI should use the UIWheelEvent type for consistency
TypeScript won't catch if the wheel event emitter forgets to include coords

Suggested fix: Change UI.ts:133 to use the shared type:

onWheel?: (e: UIWheelEvent) => void;

2. scrollHandler cleanup function is fragile (low-medium severity)

scrollHandler.ts:105-114 chains handlers by capturing prevHandler and replacing rootUI.onWheel. If two scroll handlers are attached and the first one's cleanup is called after the second is attached, it silently drops the second handler. This is a classic listener-chain bug. Consider using an event emitter pattern or an array of handlers instead.

3. Error entries in image cache never expire (low severity)

imageCache.ts:87-88 — Once an image fails to load, its URL is permanently marked as "error" with no retry mechanism. If a CDN has a transient failure, every image that failed during that window stays broken until a full page reload. Consider adding a retry-after timestamp or TTL for error entries.

Performance

4. No scroll throttling (medium severity)

scrollHandler.ts calls scrollView.scrollX/scrollY setters on every wheel event, and each setter triggers this.ui?.redraw() (UIView.ts:781,791). On a high-frequency trackpad/mouse, this can mean 60+ redraws per second. Consider debouncing or using requestAnimationFrame to batch scroll updates.

5. loadCachedImage called redundantly in measure function (low severity)

UIImage.ts:269 — measureImageFunc() calls loadCachedImage again on every Yoga measure pass, even if the image is already loaded. While the cache makes this cheap (Map lookup), the callback closure () => { this.yogaNode?.markDirty(); this.ui?.redraw(); } is recreated each time. A minor optimization would be to cache the callback or only register it once.

6. evictIfNeeded is O(n) per eviction (low severity)

imageCache.ts:43-54 — The eviction loop shifts from the front of insertionOrder (O(n) for arrays) and may rotate loading entries to the back. For a cache of 256 entries this is fine, but if MAX_CACHE_SIZE ever grows, consider a doubly-linked list or similar structure.

Code Quality

7. currencyUtils.ts is over-engineered for the current use case (observation)

The new currency utilities file (~430 lines) introduces a full multi-currency system (gold, silver, copper, gems, tokens) with conversion rates, breakdowns, and change indicators. However, the actual consumers (DuelPanel, TradePanel) only use formatGoldValue() which is a simple standalone function at the bottom. The rest of the infrastructure appears unused. Per the project's "production code only, no speculative abstractions" rule, consider whether the full currency system is needed now or if it should be deferred until there's a consumer.

8. isEdge duplicated between UIView.ts and UIImage.ts (low severity)

Both UIView.ts:966-973 and UIImage.ts:713-721 define identical isEdge() helper functions. Extract to a shared utility (e.g., alongside the yoga helpers).

9. UIImage.mount() silently fails on server (observation)

UIImage.ts:161 — Returns early with no logging when !isBrowser, while UIView.ts:227 checks this.ctx?.network.isServer. These should use a consistent pattern for server-side early returns.

Security

10. No URL validation on image src (low severity)

imageCache.ts accepts any string as a URL and creates an Image element with it. While the crossOrigin = "anonymous" attribute prevents credential leakage, there's no scheme validation. A javascript: URL would be a no-op for Image, but data: URLs could be used for cache-stuffing attacks. If src values come from user-generated content (e.g., custom avatars), consider validating the URL scheme.

Test Coverage

11. No tests for new features

The new imageCache, UIImage, and scrollHandler modules have no test coverage. Per the project's testing philosophy, these should have Playwright-based tests verifying:

Image loading and cache eviction behavior
Object-fit rendering modes (at least contain and cover)
Scroll behavior with overflow content
Edge cases: failed image loads, rapid src changes, unmount during loading

What Looks Good

Dead code removal is thorough — all 7 deleted files have zero remaining imports
Type consolidation is well-organized — game/types/ barrel with equipment.ts, events.ts, ui.ts is clean
computeObjectFit is correct — all 5 CSS object-fit modes are properly implemented with correct math
Commit organization — each commit is atomic and well-scoped (one concern per commit)
UIView overflow/scroll implementation — the canvas clip + scroll offset approach is the right pattern for Yoga-based UI
SettingsPanel directory restructuring — good preparatory refactor for future split

Summary

The cleanup and consolidation work is high quality and ready to merge. The new Yoga UI features (UIImage, scroll, image cache) are well-implemented but have a few type mismatches and missing edge-case handling that should be addressed. The onWheel type mismatch (item 1) is the most important fix. Items 4 and 7 are worth discussing but not blocking.

- UI.onWheel: use UIWheelEvent type instead of inline shape - imageCache: add 30s TTL for error entries so transient CDN failures auto-retry instead of staying broken for the session - scrollHandler: batch wheel deltas with requestAnimationFrame to avoid redundant redraws from high-frequency trackpad events; cancel rAF on cleanup - isEdge: deduplicate into extras/ui/yoga.ts, import from UIView + UIImage - UIImageData: remove | string from width/height (impl expects number)

github-actions · 2026-04-11T16:45:41Z

Code Review: PR #1145 — Client Cleanup, Dead Code Removal, Shared Exports & Yoga UI

Good cleanup PR overall — removing ~2,500 lines of dead code and consolidating duplicated types is solid housekeeping. The new UIImage node is substantial and well-structured. A few issues to flag:

Bugs / Issues

1. Callback accumulation in imageCache.ts (medium severity)

loadCachedImage() is called on every draw() cycle in UIImage.ts:123, and each call pushes a new callback into the loading entry's callbacks array (imageCache.ts:88). If an image takes 3 seconds to load and the UI redraws at 60fps, ~180 duplicate () => this.ui?.redraw() callbacks accumulate and all fire when the image loads — a burst of redundant redraws.

Suggestion: guard against duplicate registration, e.g. return early if the entry is already loading and the caller already has a pending callback, or deduplicate callbacks.

2. Scroll handler content size calculation incorrect for row layouts (scrollHandler.ts:63-76)

contentHeight unconditionally sums all children's heights and contentWidth sums all children's widths. This is only correct for column (vertical) flex direction. For flexDirection: "row", content height should be the max child height (not sum), and content width should be the sum. Currently horizontal scrolling in a row layout would have incorrect max-scroll bounds.

3. formatGoldValue behavioral change for DuelPanel

The old DuelPanel formatGoldValue used toFixed(1) (e.g. 1050 → "1.1K"), while the consolidated version uses floor-based remainder (e.g. 1050 → "1K"). This is a minor display regression for the DuelPanel. If this is intentional, no action needed — but worth calling out.

Code Quality

4. Unused import in UIImage.ts:11

every is imported from lodash-es but never used anywhere in the file. Will likely trip the lint check.

// UIImage.ts:11
import { every, isArray, isBoolean, isNumber, isString } from "lodash-es";
//       ^^^^^ unused

5. UIImage doesn't cancel in-flight loads on unmount() (UIImage.ts:217-228)

unmount() clears yogaNode and box but not this.ui. After unmount, the loadCachedImage callback (() => this.ui?.redraw()) can still fire and trigger a redraw on the parent UI that no longer contains this node. Not a crash (optional chaining saves it), but causes unnecessary work. Consider either clearing this.ui on unmount or tracking a _mounted flag.

6. Scroll handler cleanup has stale-closure risk (scrollHandler.ts:125-135)

The cleanup function captures prevHandler at attachment time and restores it on cleanup. If another handler is attached after this one, calling cleanup will overwrite that newer handler. This is a handler chain corruption bug in non-LIFO detachment order. Consider checking if rootUI.onWheel is still the handler you installed before restoring.

Minor / Nits

7. Missing deltaMode normalization (scrollHandler.ts)

The UIWheelEvent includes deltaMode (0=pixels, 1=lines, 2=pages per DOM spec), but the scroll handler treats all deltas as raw pixel values. If deltaMode is 1 or 2, scroll speed will be unexpectedly slow.

8. overflow setter in UIView.ts doesn't validate input

Unlike other setters in the class, the overflow setter accepts arbitrary strings. An invalid value like "auto" would set _overflow to "auto", which makes needsClip true (since it's not "visible") but doesn't correspond to any real Yoga overflow mode. Consider validating against "visible" | "hidden" | "scroll".

What Looks Good

All deletions are safe — verified no dangling imports for CurvePane, CurvePreview, Fields, useQuestLog, websocket-manager, player.ts, or rendering/ui.ts
Type consolidation is correct — accessibility and complexity types in shared match the old client-only definitions exactly
Import path updates are consistent across all DuelPanel and TradePanel files
Build script fix for framework.client.d.ts is the right approach for the split client/server exports
Image cache design is sound — LRU eviction, error TTL, callback coalescing, proper cleanup API
UIImage node is well-implemented with correct Canvas state management, object-fit modes, and border radius support

Nice work on the cleanup. Items 1-3 are worth addressing before merge; the rest are minor.

1. imageCache: deduplicate callbacks by reference — UIImage now caches its redraw callback as a stable property so includes() prevents accumulation from repeated draw() cycles 2. scrollHandler: compute content size correctly for row layouts — sum widths/max height for row, sum heights/max width for column 3. formatGoldValue: revert to toFixed(1) for all tiers (K/M/B) to match the original DuelPanel behavior exactly 4. UIImage: remove unused `every` lodash import (was for old isEdge) 5. UIImage: clear ui reference on unmount so in-flight imageCache callbacks don't trigger redraws on detached nodes; add _mounted flag 6. scrollHandler: guard cleanup against stale closures — only restore prevHandler if we're still the active handler 7. scrollHandler: normalize deltaMode (lines=16px, pages=viewport) per DOM WheelEvent spec 8. UIView: validate overflow setter input against "visible" | "hidden" | "scroll", warn and default to "visible" for invalid values

github-actions · 2026-04-11T17:11:32Z

Code Review: PR #1145 — Client Cleanup, Dead Code Removal, Shared Exports & Yoga UI

Overall this is a well-structured cleanup PR. The dead code removal is thorough and verified — all deleted files have zero remaining references. The type consolidation is correct and maintains API compatibility. The new Yoga UI features (UIImage, scroll, overflow) are well-engineered. A few items worth addressing:

Bug: Unused `every` import in UIView.ts

packages/shared/src/nodes/UIView.ts:7 — After moving isEdge to yoga.ts, the every import from lodash-es is now dead:

import { every, isArray, isBoolean, isNumber, isString } from "lodash-es";
//       ^^^^^  no longer used anywhere in this file

This will likely trigger a lint error. Remove every from the import.

Nit: Dead `_mounted` field in UIImage.ts

packages/shared/src/nodes/UIImage.ts:76 — _mounted is set to true in mount() and false in unmount() but is never read anywhere. Either remove it or use it (e.g., as a guard in draw() or as part of the _onImageLoad callback safety check). Currently ui being undefined serves as the unmount guard, making _mounted redundant.

Design: Inconsistent error handling in UIView `overflow` setter

packages/shared/src/nodes/UIView.ts — The overflow setter uses console.warn + default-to-visible for invalid values:

set overflow(value: OverflowMode) {
    if (value !== "visible" && value !== "hidden" && value !== "scroll") {
      console.warn(`[UIView] Invalid overflow value "${value}", defaulting to "visible"`);
      value = "visible";
    }

Every other setter in UIView and UIImage throws an Error on invalid input (e.g., throw new Error("[uiview] scrollX not a number")). For consistency, this should throw too. Silent coercion can mask bugs.

Nit: Loose `margin` type in `UIImageData`

packages/shared/src/types/rendering/nodes.ts — UIImageData.margin is typed as number | number[] | null but the runtime validation (isEdge) requires exactly a 4-element [number, number, number, number] tuple for arrays. Consider tightening the type:

margin?: number | [number, number, number, number] | null;

This matches the isEdge return type in yoga.ts and would catch incorrect array lengths at compile time.

Minor: Variable shadowing in scrollHandler.ts

packages/shared/src/extras/ui/scrollHandler.ts — In the handler function, width and height are destructured at the top of the function from scrollView.box, then again in the deltaMode === 2 branch:

const { left, top, width, height } = scrollView.box;
// ...
if (event.deltaMode === 2) {
  const { height, width } = scrollView.box;  // shadows outer

This is functionally correct (same source object) but could confuse readers. Since both refer to scrollView.box, the inner destructure is unnecessary — just use the outer variables.

Behavioral change: `formatGoldValue` in TradePanel

The old TradePanel formatGoldValue used Math.floor for K/M/B values (e.g., 1999 → "1K"). The new canonical version in currencyUtils.ts uses toFixed(1) (e.g., 1999 → "2.0K"). This is arguably an improvement and matches the DuelPanel's previous behavior, but worth noting as a visible display change.

Cleanup opportunity: Orphaned hooks

usePane (packages/client/src/hooks/usePane.tsx) and useUpdate (packages/client/src/hooks/useUpdate.ts) lost their only consumers (CurvePane.tsx and Fields.tsx) and are now dead code. Not blocking, but a good follow-up cleanup.

Positive highlights

No any types — fully compliant with the project's strict typing rules
Image cache design is well-thought-out: coalesced callbacks, LRU eviction that skips loading entries, error TTL for retry, SSR-safe isBrowser guards
computeObjectFit correctly implements all five CSS object-fit modes
_onImageLoad callback deduplication prevents callback accumulation from repeated draw() cycles — good attention to the draw-loop performance pattern
attachScrollHandler rAF batching prevents redundant redraws from high-frequency wheel/trackpad events
isEdge refactoring from UIView-local to shared yoga.ts utility is a solid DRY improvement
Client-specific .d.ts generation (framework.client.d.ts) is a correctness fix — prevents server-only types from leaking into client typechecking
Dead code verification is thorough — all ~2,500 deleted lines are confirmed unreferenced

Nice work on the cleanup. The one item I'd flag as worth fixing before merge is the dead every import (likely lint failure). The rest are nits and suggestions.

- UIView: remove dead `every` lodash import (moved isEdge to yoga.ts) - UIImage: remove unused `_mounted` field (ui=undefined is the guard) - UIView: overflow setter throws on invalid input (consistent with all other setters in the class) - UIImageData: tighten margin type to [number,number,number,number] tuple (matches isEdge validation) - scrollHandler: remove variable shadowing in deltaMode=2 branch

github-actions · 2026-04-11T17:20:28Z

Code Review: PR #1145 — Client Cleanup, Dead Code Removal, Shared Exports & Yoga UI

Excellent cleanup PR. The dead code removal is well-researched (commit messages cite import counts and verify no consumers), the type consolidation is sound, and the new UIImage/scroll features are solid additions. A few findings below, roughly ordered by severity.

Issues to Address

1. measureImageFunc() creates a new callback on every invocation — defeats imageCache deduplication
UIImage.ts:274 — The Yoga measure function creates a fresh () => { this.yogaNode?.markDirty(); this.ui?.redraw(); } closure each time it's called. Since imageCache deduplicates callbacks by reference (includes()), this means repeated layout passes will accumulate duplicate callbacks. The draw() path correctly uses the cached _onImageLoad property — the measure path should do the same or use a second stable callback cached on the instance.

2. UIImageData.display typed as string instead of DisplayType
nodes.ts:716 — The UIImageData interface types display as string, while UIViewData correctly uses DisplayType. This weakens type safety; consumers can pass arbitrary strings. Should be display?: DisplayType.

3. UIViewData.overflow duplicates the OverflowMode type inline
nodes.ts:707 — overflow?: "visible" | "hidden" | "scroll" is defined inline on the interface, and the same union is exported separately as OverflowMode on line 712. Should use overflow?: OverflowMode for DRY.

4. scrollHandler content-size calculation ignores padding
scrollHandler.ts:74-92 — The content size is computed as the sum of child sizes plus margins, but the scroll view's own padding is not subtracted from the viewport area (or added to content). If the scrollView has padding, the scroll range will be slightly off, causing content to be clipped early or over-scroll.

5. scrollHandler recalculates content size on every wheel flush
scrollHandler.ts:74-92 — For scroll containers with many children, iterating all children to compute content size on every animation frame is potentially expensive. Consider caching the content size and invalidating on layout changes (e.g., when yogaNode.markDirty() is called), or at minimum documenting that this is a known trade-off for simplicity.

Minor / Suggestions

6. imageCache insertionOrder.indexOf(url) is O(n)
imageCache.ts:100 — On error-TTL expiry, indexOf scans the insertion order array. For the 256-entry cap this is fine in practice, but a Set or secondary Map would make it O(1) if the cache size grows.

7. No URL validation in imageCache
imageCache.ts:78-108 — loadCachedImage accepts any string as a URL and creates an <img> element with it. While the crossOrigin = "anonymous" is good, consider validating that URLs are HTTP(S) to avoid javascript: or other protocol injection if user-generated content ever flows through this path. The doc comment says "pre-resolved HTTP URL" — a runtime guard would back that up.

8. Missing isOverflow() validator in yoga.ts
yoga.ts — Every other layout property (display, flexDirection, justifyContent, alignItems, etc.) has a corresponding isX() guard function. Overflow doesn't, so the UIView setter manually checks against the three valid strings. Adding isOverflow() would be consistent with the existing pattern.

9. formatGoldValue potential behavioral difference
The consolidated formatGoldValue in currencyUtils.ts:364-373 uses toFixed(1) for K/M/B tiers and toLocaleString() for values < 1000. The old TradePanel/utils.ts version used toFixed(0) for the K tier. The commit message says this was intentionally reverted to toFixed(1) to match the DuelPanel — just flagging to confirm this is the intended display for the TradePanel too (e.g., "1.5K" instead of "2K" for 1500).

What Looks Good

Dead code removal is thorough and well-justified: Each deletion commit cites zero importers and names the production replacement (e.g., useQuestLog vs questStore.ts + useQuestStatusSync, WebSocketManager vs world.init() networking). This is the right way to do cleanup.
Type consolidation into game/types/ with types/index.ts re-exporting is clean. Single source of truth without breaking existing import paths.
formatGoldValue consolidation properly centralizes the three duplicate definitions and updates all consumers.
SettingsPanel directory restructuring resolves the file-vs-directory naming conflict cleanly.
UIImage implementation is well-structured: computeObjectFit correctly implements all five CSS object-fit modes, the _onImageLoad callback caching for the draw path prevents accumulation, unmount() clears this.ui to prevent stale redraws, copy() deep-copies the margin array, and the proxy properly inherits Node properties.
UIView overflow/scroll implementation is sound: Yoga OVERFLOW_HIDDEN for both "hidden" and "scroll" (with scroll handled in canvas drawing), proper ctx.save()/clip()/restore() lifecycle, resolution-scaled scroll offsets, and correct roundRect clip paths.
imageCache has good design choices: coalesced callbacks, 30s error TTL for transient failures, LRU eviction that skips loading entries, try/catch around callbacks, and server-safe typeof window guard.
scrollHandler has smart details: rAF batching of wheel deltas, deltaMode normalization per DOM spec, pointer-in-bounds check, safe cleanup that checks handler identity before restoring.
Build script change correctly generates framework.client.d.ts to match the package.json ./client export, with a graceful warning if the source declaration is missing.
@internal JSDoc tags on UI node exports in index.client.ts are appropriate — these are engine-internal 3D canvas nodes, not for client React/CSS consumption.

Overall this is a well-executed cleanup with good new features. The main actionable item is #1 (measure callback accumulation). Nice work.

SYMBaiEX added 18 commits April 10, 2026 13:56

refactor(client): remove dead Fields, CurvePane, CurvePreview components

ce5d94a

Delete ~950 LOC of unreferenced field editor components. Fields.tsx defined 10 React field components never imported by any consumer. CurvePane.tsx and CurvePreview.tsx were only imported by Fields.tsx.

refactor(client): remove unused HintContextType from types barrel

1d3e8b7

The exported HintContextType had zero importers. The production HintContext lives in ui/components/Hint.tsx with its own local type.

feat(shared): export accessibility, complexity, and combat-level utils

e7e0ead

Wire up orphaned shared types that were built for client consumption but never exported from the package. Includes COLORBLIND_PALETTES, COMPLEXITY_MODE_CONFIGS, getCombatLevelColor, and related types.

Copilot AI review requested due to automatic review settings April 10, 2026 18:58

SYMBaiEX requested review from dreaminglucid and lalalune as code owners April 10, 2026 18:58

SYMBaiEX mentioned this pull request Apr 10, 2026

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

Open

6 tasks

Copilot started reviewing on behalf of SYMBaiEX April 10, 2026 18:59 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

SYMBaiEX added 2 commits April 11, 2026 11:07

Merge remote-tracking branch 'origin/main' into sym/pr1-cleanup-exports

62c22bf

Conversation

SYMBaiEX commented Apr 10, 2026

Summary

Test plan

Uh oh!

github-actions Bot commented Apr 10, 2026

PR Review: refactor: client cleanup, dead code removal, shared exports & Yoga UI

Bug: UIView.copy() missing new properties

Bug: imageCache.ts LRU eviction can leave cache oversized

Type consistency: UIViewData.overflow vs OverflowMode

Unsafe casts in scrollHandler.ts

imageCache: error entries never expire

Nits (non-blocking)

What looks good

Uh oh!

github-actions Bot commented Apr 10, 2026

PR Review: refactor: client cleanup, dead code removal, shared exports & Yoga UI

Overall Assessment

Positive

Issues & Suggestions

1. formatGoldValue — Subtle behavioral change for DuelPanel consumers (minor)

2. UIImage — this.ui! non-null assertions in setters (medium)

3. scrollHandler.ts — Fragile internal API access (minor)

4. imageCache.ts — Eviction can stall on loading entries (minor)

5. DuelArenaVisualsSystem.ts & EquipmentVisualSystem.ts — Unrelated import additions (nit)

6. No tests for new UIImage/scroll functionality

Security

Performance

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 11, 2026

Code Review: PR #1145

Overall Assessment

Bugs / Correctness

Type Safety

Performance

Minor Suggestions (non-blocking)

What Looks Good

Uh oh!

github-actions Bot commented Apr 11, 2026

PR Review: refactor: client cleanup, dead code removal, shared exports & Yoga UI

Bugs

Performance

Code Quality

Security

Test Coverage

What Looks Good

Summary

Uh oh!

github-actions Bot commented Apr 11, 2026

Code Review: PR #1145 — Client Cleanup, Dead Code Removal, Shared Exports & Yoga UI

Bugs / Issues

Code Quality

Minor / Nits

Bug: `UIView.copy()` missing new properties

Bug: `imageCache.ts` LRU eviction can leave cache oversized

Type consistency: `UIViewData.overflow` vs `OverflowMode`

Unsafe casts in `scrollHandler.ts`

1. `formatGoldValue` — Subtle behavioral change for DuelPanel consumers (minor)

2. `UIImage` — `this.ui!` non-null assertions in setters (medium)

3. `scrollHandler.ts` — Fragile internal API access (minor)

4. `imageCache.ts` — Eviction can stall on loading entries (minor)

5. `DuelArenaVisualsSystem.ts` & `EquipmentVisualSystem.ts` — Unrelated import additions (nit)

Bug: Unused `every` import in UIView.ts

Nit: Dead `_mounted` field in UIImage.ts

Design: Inconsistent error handling in UIView `overflow` setter

Nit: Loose `margin` type in `UIImageData`

Behavioral change: `formatGoldValue` in TradePanel