refactor(ui): use asEntityId helper at piece-id boundaries
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
parent
567480a788
commit
9960ea96cf
6 changed files with 79 additions and 7 deletions
61
.sisyphus/notepads/polish-t2/learnings.md
Normal file
61
.sisyphus/notepads/polish-t2/learnings.md
Normal file
|
|
@ -0,0 +1,61 @@
|
|||
# Polish-T2 — Notepad
|
||||
|
||||
Scope: Option C cleanup after modifier-profiles-t2 ships. Touches modifier internals + UI + a small solo regression guard. Three commits max.
|
||||
|
||||
## Baseline (master @ 567480a)
|
||||
|
||||
- `bun run check` green: 1231 unit tests, 0 lint errors.
|
||||
- `bunx playwright test` green: 58/58.
|
||||
- Strict TS: no `as any` is a lint error. `as unknown as X` is allowed by the compiler but we're cleaning ours up because each one was flagged in T2 final audit as lazy typing.
|
||||
|
||||
## Key facts the polish relies on
|
||||
|
||||
### `as unknown as` sites in scope (5 targets)
|
||||
|
||||
1. `packages/chess/src/modifiers/registry.ts:49` — `this.#byId.set(descriptor.id, descriptor as unknown as ModifierDescriptor)`. Registry is a `Map<Id, ModifierDescriptor>` (unknown-widened). The generic `register<V>(descriptor: ModifierDescriptor<V>)` accepts typed descriptors; the store erases V. Fix: widen the parameter up-front via an explicit typed conversion that doesn't need `unknown`: e.g. accept as-is but annotate the Map value type with a stored intersection, or just do `const widened: ModifierDescriptor = descriptor;` — TS should accept this assignment because `ModifierDescriptor<V>` is assignable to `ModifierDescriptor<unknown>` only via its covariant `describe`/`apply`, which are actually contravariant in V → so direct assignment FAILS the check. Cleanest fix: change the generic signature to `register(descriptor: ModifierDescriptor)` with no generic (since V is immediately erased anyway), and have callers rely on `MODIFIER_REGISTRY.register(FOO_DESCRIPTOR)` implicitly widening `ModifierDescriptor<number>` → `ModifierDescriptor<unknown>`. But this ALSO fails because of contravariance. RIGHT fix: introduce an internal stored type `ModifierDescriptorAny` with `value: unknown` params and use a small `toStored(d)` converter that does the widening in one place with `// eslint-disable-next-line` if the compiler balks — OR rethink the descriptor's `apply`/`describe` to take `unknown` and let each descriptor narrow via a Zod parse inside. Pragmatically: take `descriptor as ModifierDescriptor` (single cast, not double) — that probably works because `ModifierDescriptor` (no-arg default) = `ModifierDescriptor<unknown>` and `ModifierDescriptor<number>` isn't structurally assignable to it, so a one-step cast is required. Accept that one cast; lose the `unknown` interstitial.
|
||||
|
||||
2. `packages/chess/src/modifiers/schema.ts:88` — `return ModifierProfileSchema.parse(raw) as unknown as ModifierProfile`. The schema types out to a plain object mirror of ModifierProfile but the nested `readonly` and branded differences trip up assignment. Fix: add `.transform((v): ModifierProfile => v as ModifierProfile)` on the schema, or use `z.custom<ModifierProfile>` at the top, or define `ModifierProfile` from `z.infer<typeof ModifierProfileSchema>` instead of the hand-rolled interface in `types.ts`. Simplest: a single `as ModifierProfile` post-parse, no `unknown` bridge.
|
||||
|
||||
3. `packages/chess/src/ui/ModifierTooltip.tsx:23`, `ui/ModifiedPieceIndicator.tsx:12`, `ui/ModifierPinnedPanel.tsx:29` — all of the form `pieceId as unknown as EntityId`. `EntityId = number & { readonly __brand: "EntityId" }`. The UI passes `number` because the `PieceState` at `Board.tsx:50-54` uses `id: number`. Fix: introduce a `toEntityId(n: number): EntityId` helper (single cast site, documented) OR change `PieceState.id` + the prop type to `EntityId`. The helper is smaller-surface. Place it in `packages/rete/src/schema.ts` alongside the type or in `packages/chess/src/modifiers/source.ts` (already re-imports EntityId from @paratype/rete). Actually `packages/rete` already has internal helpers like `mkId(n)` used only in tests; exporting a public one is the right move. Put it next to the type: `export const asEntityId = (n: number): EntityId => n as EntityId` with a doc comment explaining when to use it ("trust boundary: you've already verified this number came from Session.nextId or an EAV fact; otherwise use `session.allFacts()` to find a real one").
|
||||
|
||||
### Test-only casts NOT in scope (keep as-is)
|
||||
|
||||
- `source.test.ts:29/40`, `validate.test.ts:230`, `net/prediction.test.ts:44/73`, `net/client.test.ts:156`, `presets/*.test.ts` — these are test mocks / fixtures. Not cleaning up casts in tests as part of this polish (they'd need different mocks; scope creep).
|
||||
- `engine.ts:268` — `value as unknown as import("@paratype/rete").FactValue` — different subsystem (Rete fact value widening), not flagged in T2 audit. Leave alone.
|
||||
- `net/client.ts:182/190` — listener type widening for the subscriber bus. Not flagged. Leave alone.
|
||||
|
||||
### `getModifierSource` attr-aliasing (src/modifiers/source.ts:59-61)
|
||||
|
||||
Current hardcoded ladder:
|
||||
```
|
||||
if (attrName === "HpBonus") attrName = "Hp";
|
||||
else if (attrName === "RangeBonus") attrName = "Range";
|
||||
```
|
||||
Semantically: "this modifier augments the preset-declared base attribute". `HpBonus` augments `Hp` (declared by `piece-hp` preset). `RangeBonus` has no corresponding declared base attribute — `Range` isn't in `ChessAttrMap` at all. The `"Range"` branch is dead code today; it only matters IF a future preset declares `Range` in its `pieceAttributes`.
|
||||
|
||||
Cleanest fix: add an optional `baseAttr?: ChessAttrKey` to `ModifierDescriptor`. `hp-bonus.ts` declares `baseAttr: "Hp"`. `range-bonus.ts` leaves it undefined (or adds it when/if a preset declares Range). Then `getModifierSource` checks `descriptor.baseAttr ?? descriptor.attrName` against preset `pieceAttributes` and the aliasing vanishes.
|
||||
|
||||
Watch out: `ModifierDescriptor` is in `types.ts`; adding an optional field is non-breaking. Test file at `registry.test.ts:26-39` constructs a mock descriptor with no `baseAttr` — optional means the test keeps working.
|
||||
|
||||
### Solo-mode modifier badges
|
||||
|
||||
Current state (verified post-T2):
|
||||
- `Lobby.resetToFreshGame` (line 219-238) already forwards `selectedProfile` to `new ChessEngine({ layout, profile })` for solo — T2 fix from `567480a`/`980d567`. So the solo engine HAS the profile and the facts applied.
|
||||
- `Board.tsx:354-356` renders `<ModifiedPieceIndicator pieceId={piece.id} engine={engine}>` unconditionally when `engine !== undefined`.
|
||||
- `GameView` (solo path, `engineState` omitted) → `GameLayout` → passes `state.engine` to `<Board engine={...}>`.
|
||||
|
||||
Likely already works end-to-end! The "ship solo badges" task is mostly a regression-test addition: add a Playwright test that selects a profile in the lobby, clicks Play Solo, and asserts `[data-testid^="modifier-indicator-"]` is visible. If the test fails, then we debug; otherwise, just land the test as the guard.
|
||||
|
||||
Probable file: add test to `solo-smoke.spec.ts` (T2 added it as the canary) or extend `modifier-profiles.spec.ts` with a solo variant. Prefer `solo-smoke.spec.ts` — that's where solo invariants live.
|
||||
|
||||
### Commands
|
||||
|
||||
- `bun run check` — typecheck + lint + vitest
|
||||
- `bunx playwright test --reporter=list` — full e2e
|
||||
- `bunx playwright test e2e/solo-smoke.spec.ts --reporter=list` — solo only
|
||||
- WS server for e2e multiplayer tests: `tmux new-session -d -s ws-server 'bun run packages/server/src/index.ts'` (not needed for solo tests)
|
||||
|
||||
## [2026-04-19 13:53] Task: commit-1 verification gate
|
||||
|
||||
- Gotcha: `bun run build` (tsup/vite) cleaned package `dist/` outputs and removed TS project-reference declarations; root `bun run check` then failed with TS6305 + missing `@paratype/rete` declarations.
|
||||
- Resolution for this session: regenerate declaration outputs with `bunx tsc -b --force packages/rete packages/chess` before running the check gate; afterward `bun run check` returned PASS.
|
||||
|
|
@ -1,6 +1,6 @@
|
|||
import { MODIFIER_REGISTRY } from '../modifiers/registry';
|
||||
import type { ChessEngine } from '../engine';
|
||||
import type { EntityId } from '@paratype/rete';
|
||||
import { asEntityId } from '@paratype/rete';
|
||||
|
||||
interface Props {
|
||||
pieceId: number;
|
||||
|
|
@ -8,8 +8,9 @@ interface Props {
|
|||
}
|
||||
|
||||
export function ModifiedPieceIndicator({ pieceId, engine }: Props) {
|
||||
const id = asEntityId(pieceId);
|
||||
const isModified = MODIFIER_REGISTRY.list().some((descriptor) => {
|
||||
return engine.session.get(pieceId as unknown as EntityId, descriptor.attrName) !== undefined;
|
||||
return engine.session.get(id, descriptor.attrName) !== undefined;
|
||||
});
|
||||
|
||||
if (!isModified) {
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@
|
|||
* state on each render so it stays reactive to fact changes (moves, rule
|
||||
* changes, etc.) without any additional subscription setup.
|
||||
*/
|
||||
import type { EntityId } from '@paratype/rete';
|
||||
import { asEntityId } from '@paratype/rete';
|
||||
import { MODIFIER_REGISTRY } from '../modifiers/index.js';
|
||||
import { getModifierSource } from "../modifiers/source.js";
|
||||
import type { ChessEngine } from '../engine.js';
|
||||
|
|
@ -26,7 +26,7 @@ export function ModifierPinnedPanel({ pieceId, engine, onClose }: Props) {
|
|||
if (pieceId === null || engine === null) return null;
|
||||
|
||||
const { session } = engine;
|
||||
const id = pieceId as unknown as EntityId;
|
||||
const id = asEntityId(pieceId);
|
||||
|
||||
// Basic piece facts — bail if the entity doesn't exist in session.
|
||||
const pieceType = session.get(id, 'PieceType') as string | undefined;
|
||||
|
|
|
|||
|
|
@ -9,7 +9,7 @@
|
|||
* MODIFIER_REGISTRY attribute is shown without distinguishing per-type /
|
||||
* per-instance / preset origin (T26 can refine this).
|
||||
*/
|
||||
import type { EntityId } from '@paratype/rete';
|
||||
import { asEntityId } from '@paratype/rete';
|
||||
import { MODIFIER_REGISTRY } from '../modifiers/index.js';
|
||||
import type { ChessEngine } from '../engine.js';
|
||||
|
||||
|
|
@ -20,7 +20,7 @@ interface Props {
|
|||
|
||||
export function ModifierTooltip({ pieceId, engine }: Props) {
|
||||
const { session } = engine;
|
||||
const id = pieceId as unknown as EntityId;
|
||||
const id = asEntityId(pieceId);
|
||||
|
||||
// Basic piece facts
|
||||
const pieceType = session.get(id, 'PieceType') as string | undefined;
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ export type {
|
|||
SchemaKind,
|
||||
DefineSchemaOptions,
|
||||
} from "./schema.js";
|
||||
export { defineSchema, fact } from "./schema.js";
|
||||
export { asEntityId, defineSchema, fact } from "./schema.js";
|
||||
|
||||
export type { AttrKey, FactValue } from "./wm.js";
|
||||
export { WorkingMemory } from "./wm.js";
|
||||
|
|
|
|||
|
|
@ -24,6 +24,16 @@
|
|||
*/
|
||||
export type EntityId = number & { readonly __brand: "EntityId" };
|
||||
|
||||
/**
|
||||
* Coerce a raw number to the branded EntityId type. Use ONLY at trust
|
||||
* boundaries where you've already established that `n` came from
|
||||
* `Session.nextId()` or a fact's `id` field (e.g. UI code that carries
|
||||
* a piece's id as `number` because React prop serialization doesn't
|
||||
* preserve branding). Prefer passing `EntityId` through end-to-end when
|
||||
* possible; this helper is the single legitimate cast site.
|
||||
*/
|
||||
export const asEntityId = (n: number): EntityId => n as EntityId;
|
||||
|
||||
/**
|
||||
* Shape of a schema type parameter.
|
||||
*
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue