AI Implementation feature(912): Scoreboard: Ranking, Matrix, Event Log and Score Graph 1.02 (#55)
This commit was merged in pull request #55.
This commit is contained in:
@@ -1,202 +0,0 @@
|
||||
# Implementation Plan: Scoreboard — Distinct Line Colors per Top-10 Player on Score Graph
|
||||
|
||||
## Status
|
||||
|
||||
**Feature NOT yet implemented.** The `PLAYER_COLOR_PALETTE` exposes 10 distinct
|
||||
hex colors and there are 10 legend entries / 10 polylines, but each `colorIndex`
|
||||
is derived from `stablePlayerColorIndex(playerId)` — an FNV-1a hash modulo 10.
|
||||
For many player-id sets (including the reported one) that hash produces
|
||||
collisions: e.g. `PlayerC` and `Player05` both hash to `5` → `#ec4899`;
|
||||
`Player07` and `Player08` both hash to `9` → `#84cc16`; `PlayerA` and
|
||||
`Player10` both hash to `2` → `#10b981`. Verified empirically:
|
||||
|
||||
```
|
||||
hash → palette index
|
||||
PlayerA → 2 (#10b981) green
|
||||
PlayerB → 8 (#6366f1)
|
||||
PlayerC → 5 (#ec4899) pink ← collides with Player05
|
||||
Player04 → 8 (#6366f1)
|
||||
Player05 → 5 (#ec4899) pink ← collides with PlayerC
|
||||
Player06 → 3 (#ef4444)
|
||||
Player07 → 9 (#84cc16) lime ← collides with Player08
|
||||
Player08 → 9 (#84cc16) lime ← collides with Player07
|
||||
Player09 → 3 (#ef4444)
|
||||
Player10 → 2 (#10b981) green ← collides with PlayerA
|
||||
```
|
||||
|
||||
Collisions are observed across **all four** scoreboard tabs (Ranking swatch,
|
||||
Matrix first-column swatch, Event Log has no swatch, Score Graph swatch +
|
||||
polyline) because every component reads its color through the same
|
||||
`colorIndex → PLAYER_COLOR_PALETTE` lookup.
|
||||
|
||||
The Job text limits the symptom to the Score Graph, but the root cause is
|
||||
shared data (`colorIndex`), so the fix must be applied at the data layer
|
||||
(`scoreboard.pure.ts`) and re-projected through `scoreboard.store.ts` so the
|
||||
same top-10 player keeps the same distinct color in every tab.
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
|
||||
- **Framework & language:** TypeScript strict mode, Angular standalone
|
||||
components, NestJS backend (irrelevant for this fix — the bug is purely
|
||||
client-side rendering).
|
||||
- **Code style:** Standalone components with `ChangeDetectionStrategy.OnPush`,
|
||||
signal-based store, pure-function helpers in `scoreboard.pure.ts` (no
|
||||
Angular DI), services in `*.service.ts`, store in `*.store.ts`.
|
||||
- **Data layer:** Pure-function projections over server-supplied payloads.
|
||||
The four tabs are populated from
|
||||
`/api/v1/scoreboard/{ranking,matrix,event-log,graph}` (parallel fetch in
|
||||
`ScoreboardStore.loadAll()`) and incrementally patched in
|
||||
`ScoreboardStore.applySolveEvent()` from the `/api/v1/events` SSE
|
||||
`solve` frame.
|
||||
- **Color model:** A single readonly 10-entry hex array
|
||||
`PLAYER_COLOR_PALETTE` in `frontend/src/app/features/scoreboard/scoreboard.pure.ts:73`
|
||||
and a hashing helper `stablePlayerColorIndex(playerId)`
|
||||
(`scoreboard.pure.ts:86`). Every consumer (ranking, matrix, score-graph)
|
||||
reads `PLAYER_COLOR_PALETTE[idx]` directly. There is currently **no**
|
||||
collision-resolution pass.
|
||||
- **Test framework & structure:** Jest 29 with `ts-jest` preset, configured
|
||||
by `/repo/tests/jest.config.js` (two projects: `backend` and `frontend`,
|
||||
the latter with `jsdom`). Tests live exclusively in `/repo/tests/frontend/`
|
||||
and `/repo/tests/backend/` (never next to source). Single command:
|
||||
`npm test` from the repo root.
|
||||
- **Required tools & dependencies:** None new. The fix is purely a
|
||||
pure-function rewrite inside existing files plus a tiny test addition.
|
||||
No `setup.sh` change, no new packages.
|
||||
|
||||
## 2. Impacted Files
|
||||
|
||||
- **To Modify:**
|
||||
- `frontend/src/app/features/scoreboard/scoreboard.pure.ts` — add a
|
||||
collision-free top-N palette assigner; rewire the three projection
|
||||
helpers (`parseSolveEventIntoRanking`, `mutateMatrixFromSolve`,
|
||||
`applySolveToGraph`) so they use it instead of bare
|
||||
`stablePlayerColorIndex`.
|
||||
- `frontend/src/app/features/scoreboard/scoreboard.store.ts` — apply
|
||||
the unique-color projection after `loadAll()` populates each signal
|
||||
(ranking/matrix/graph) so the initial REST responses also benefit.
|
||||
- `tests/frontend/scoreboard.pure.spec.ts` — add focused specs covering
|
||||
the new collision-free assigner (success path: distinct indices for
|
||||
the documented colliding ID set) and a regression spec asserting
|
||||
existing single-player behaviour still holds.
|
||||
|
||||
- **To Create:** None.
|
||||
|
||||
## 3. Proposed Changes
|
||||
|
||||
### 3.1. Add a unique-color assigner in `scoreboard.pure.ts`
|
||||
|
||||
Export a new pure helper next to `stablePlayerColorIndex`:
|
||||
|
||||
```ts
|
||||
export function assignUniquePlayerColors<T extends { playerId: string }>(
|
||||
items: readonly T[],
|
||||
getColorIndex: (t: T) => number,
|
||||
capacity: number = PLAYER_COLOR_PALETTE.length,
|
||||
): T[] {
|
||||
const used = new Set<number>();
|
||||
const out: T[] = [];
|
||||
for (const item of items) {
|
||||
const preferred = ((getColorIndex(item) % capacity) + capacity) % capacity;
|
||||
let chosen = preferred;
|
||||
if (used.has(preferred)) {
|
||||
// Linear probe — the top-10 window guarantees a free slot exists.
|
||||
let probe = (preferred + 1) % capacity;
|
||||
while (used.has(probe) && probe !== preferred) {
|
||||
probe = (probe + 1) % capacity;
|
||||
}
|
||||
chosen = probe;
|
||||
}
|
||||
used.add(chosen);
|
||||
out.push({ ...item, colorIndex: chosen } as T);
|
||||
}
|
||||
return out;
|
||||
}
|
||||
```
|
||||
|
||||
Semantics:
|
||||
|
||||
- The first player keeps their hashed index; collisions are pushed to the
|
||||
next free slot via linear probe. With `capacity = 10` and at most 10
|
||||
players, a free slot is always found (the input length is itself
|
||||
bounded to ≤ capacity by the upstream `top-10 cutoff` in
|
||||
`applySolveToGraph`).
|
||||
- Order is preserved → ranking/rank-cell order is unaffected.
|
||||
- The mapper is injected so the same helper works for `RankingRow`,
|
||||
`GraphSeries`, and any future list type that needs a `colorIndex`.
|
||||
|
||||
### 3.2. Use the assigner inside the three projection helpers
|
||||
|
||||
In `parseSolveEventIntoRanking`, replace the direct assignment at the
|
||||
"new player" branch (currently
|
||||
`colorIndex: stablePlayerColorIndex(payload.playerId)`) by:
|
||||
|
||||
1. Build a temporary list of the affected/new player with the hashed index.
|
||||
2. After the sort, call `assignUniquePlayerColors(sorted, (r) => r.colorIndex)`
|
||||
and return that.
|
||||
|
||||
In `mutateMatrixFromSolve`, when a brand-new player is appended, derive
|
||||
their provisional `colorIndex` from the hash, then post-process the
|
||||
`players` array with `assignUniquePlayerColors` before returning.
|
||||
|
||||
In `applySolveToGraph`, after the new series is added and the array is
|
||||
sorted + sliced to top 10, run `assignUniquePlayerColors` on the top-10
|
||||
series so the polyline colors are guaranteed unique. New players appended
|
||||
after the cutoff don't affect the top-10.
|
||||
|
||||
This keeps the FNV hash as the **deterministic preference** (so a
|
||||
player retains their color across all tabs and across reloads whenever
|
||||
possible) while guaranteeing uniqueness inside the top-10 window.
|
||||
|
||||
### 3.3. Apply the same pass after `loadAll()` in `scoreboard.store.ts`
|
||||
|
||||
The initial REST responses already arrive with `colorIndex` set by the
|
||||
backend (which uses the same hashing convention). After the four
|
||||
parallel fetches resolve, re-project each view through the new assigner
|
||||
so the very first paint also shows distinct colors. Specifically, in
|
||||
the success branch of `loadAll()`:
|
||||
|
||||
```ts
|
||||
this._ranking.set(assignUniquePlayerColors(ranking, (r) => r.colorIndex));
|
||||
const matrixPlayers = assignUniquePlayerColors(matrix.players, (p) => p.colorIndex);
|
||||
this._matrix.set({ ...matrix, players: matrixPlayers });
|
||||
this._graph.set({ ...graph, series: assignUniquePlayerColors(graph.series, (s) => s.colorIndex) });
|
||||
```
|
||||
|
||||
(Backed by tests in §4; the projection is a no-op when there is only
|
||||
one player or when no collisions exist.)
|
||||
|
||||
### 3.4. Verify Ranking/Matrix tabs visually
|
||||
|
||||
The existing `ranking.component.ts:68`, `matrix.component.ts:108`, and
|
||||
`score-graph.component.ts:112` `color(idx)` helpers already guard
|
||||
against out-of-range indices via the same modulo trick the assigner
|
||||
uses, so no template changes are required — they keep working as soon as
|
||||
`colorIndex` is unique.
|
||||
|
||||
## 4. Test Strategy
|
||||
|
||||
- **Target test file:** `tests/frontend/scoreboard.pure.spec.ts`
|
||||
(and one regression test for `applySolveToGraph` covering the exact
|
||||
Job example: ten PlayerC/Player05-style IDs).
|
||||
- **Mocking strategy:** The helper is a pure function with no DOM, no
|
||||
Angular DI, and no async — it is imported directly into the existing
|
||||
spec file, which already mocks only `@angular/common/http` (no extra
|
||||
setup needed). No `TestBed`, no fixtures, no chart rendering.
|
||||
- **Spec list (minimal, focused):**
|
||||
1. `assignUniquePlayerColors` returns distinct indices for the
|
||||
documented colliding id set
|
||||
`['PlayerA','PlayerB','PlayerC','Player04','Player05','Player06','Player07','Player08','Player09','Player10']`
|
||||
and the colors they map to are all different (assert via
|
||||
`PLAYER_COLOR_PALETTE[idx]` set size === 10).
|
||||
2. `assignUniquePlayerColors` is a no-op when there are no collisions
|
||||
(single player, or hashed indices already unique).
|
||||
3. `applySolveToGraph` end-to-end: feed it an empty view + 10 SSE
|
||||
solves for 10 colliding players, then assert every series in the
|
||||
resulting `view.series` has a unique `colorIndex` (regression for
|
||||
the Job's DOM-inspection symptom at the polyline level).
|
||||
4. `parseSolveEventIntoRanking` regression: existing test still passes
|
||||
— proves the rewire did not change ranking sort/points logic.
|
||||
|
||||
- **How to run:** `npm test` from the repo root (resolves to the
|
||||
pre-configured Jest project; frontend project selected automatically
|
||||
because all impacted specs live under `tests/frontend/`).
|
||||
@@ -0,0 +1,129 @@
|
||||
# Implementation Plan: Scoreboard Score Graph — Safe rendering under invalid/reversed event window (Job 912)
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
|
||||
- **Codebase style & conventions:**
|
||||
- Backend: NestJS 10 + TypeORM, SQLite (`backend/src/...`).
|
||||
- Frontend: Angular 20 standalone components with Signals, `ChangeDetectionStrategy.OnPush`, `@if`/`@for` control flow, `signal()`/`computed()` for state.
|
||||
- Pure helpers live in `frontend/src/app/features/scoreboard/scoreboard.pure.ts` and are the canonical test surface; components stay presentation-only.
|
||||
- Existing `ScoreGraphComponent` uses the legacy `@Input()` + `@if/*ngIf` mix; we will not refactor it (out of scope) — only fix the broken math.
|
||||
- **Data Layer:** SQLite via TypeORM. The bug lives entirely in the frontend projection. No DB migration is required. (The root cause is bad admin-entered `eventStartUtc`/`eventEndUtc` values persisted in the `setting` table — we do NOT silently rewrite them; we harden the renderer.)
|
||||
- **Test Framework & Structure:**
|
||||
- Jest 29 with `ts-jest` and two projects in `tests/jest.config.js` (`backend` + `frontend`).
|
||||
- Frontend project uses `jsdom`, `tests/frontend/jest.setup.ts`, and `transformIgnorePatterns` allow-listing `@angular`, `rxjs`, `tslib`.
|
||||
- Tests live under `tests/frontend/` (already convention) — e.g. `tests/frontend/scoreboard.pure.spec.ts` exists and follows this pattern. We will add a sibling `tests/frontend/score-graph-projection.spec.ts`.
|
||||
- Single test command: `npm test` (or scoped `npm run test:frontend`).
|
||||
- **Required Tools & Dependencies:** None new. Existing stack (Jest 29, jsdom, `@angular/common`, `@angular/core`) is sufficient.
|
||||
|
||||
## 2. Impacted Files
|
||||
|
||||
### To Modify
|
||||
- `frontend/src/app/features/scoreboard/scoreboard.pure.ts`
|
||||
- Add a pure helper `projectGraphPoints(view, series, dims)` that returns clamped SVG `points` strings for a single series. Encapsulates the X/Y math so the bug cannot recur in the component template. Pure, deterministic, fully unit-testable.
|
||||
- Add a small helper `isValidEventWindow(startUtc, endUtc)` returning `true` only when both timestamps parse and `endMs > startMs`.
|
||||
- `frontend/src/app/features/scoreboard/score-graph.component.ts`
|
||||
- Replace inline `pointsAttr()` with a call to `projectGraphPoints(view, s, dims)` and a single `points` getter computed up front (via a `computed()` over `view`) so the bug fix is a single source of truth.
|
||||
- Switch the per-series `<polyline>` binding from a method call to a precomputed `string` per series (Angular calls methods on every change-detection cycle, so hoisting into `computed()` also helps OnPush perf).
|
||||
- No template/TDD-required structural changes beyond `[attr.points]="linePoints(s)"` consuming the hoisted value.
|
||||
|
||||
### To Create
|
||||
- `tests/frontend/score-graph-projection.spec.ts`
|
||||
- Unit tests for the new pure helper covering:
|
||||
1. Happy path (start < end, normal window) — unchanged coordinates.
|
||||
2. Reversed window (`endUtc < startUtc`) — every projected X coordinate falls inside `[padL, padL + innerW]` and no coordinate exceeds plausible SVG bounds.
|
||||
3. Equal start/end (zero span) — handled gracefully (no NaN/Infinity, all points clamp to the right edge).
|
||||
4. Invalid `Date` strings — points string is empty / coordinates stay finite.
|
||||
|
||||
## 3. Proposed Changes
|
||||
|
||||
### Step 1 — Pure helper in `scoreboard.pure.ts`
|
||||
|
||||
Add two exports:
|
||||
|
||||
```ts
|
||||
export interface GraphProjectionDims {
|
||||
width: number;
|
||||
height: number;
|
||||
padL: number;
|
||||
padR: number;
|
||||
padT: number;
|
||||
padB: number;
|
||||
}
|
||||
|
||||
export function isValidEventWindow(
|
||||
startUtc: string | null,
|
||||
endUtc: string | null,
|
||||
): boolean {
|
||||
if (!startUtc || !endUtc) return false;
|
||||
const s = new Date(startUtc).getTime();
|
||||
const e = new Date(endUtc).getTime();
|
||||
if (!Number.isFinite(s) || !Number.isFinite(e)) return false;
|
||||
return e > s;
|
||||
}
|
||||
|
||||
export function projectGraphPoints(
|
||||
view: Pick<GraphView, 'startUtc' | 'endUtc'>,
|
||||
series: GraphSeries,
|
||||
dims: GraphProjectionDims,
|
||||
): string {
|
||||
if (!isValidEventWindow(view.startUtc, view.endUtc)) return '';
|
||||
const startMs = new Date(view.startUtc!).getTime();
|
||||
const endMs = new Date(view.endUtc!).getTime();
|
||||
const span = endMs - startMs;
|
||||
const innerW = dims.width - dims.padL - dims.padR;
|
||||
const innerH = dims.height - dims.padT - dims.padB;
|
||||
|
||||
let maxVal = 0;
|
||||
for (const s of series.points) if (s.value > maxVal) maxVal = s.value;
|
||||
if (maxVal <= 0) maxVal = 1;
|
||||
|
||||
const left = dims.padL;
|
||||
const right = dims.padL + innerW;
|
||||
const top = dims.padT;
|
||||
const bottom = dims.padT + innerH;
|
||||
|
||||
return series.points
|
||||
.map((p) => {
|
||||
const ms = new Date(p.tUtc).getTime();
|
||||
if (!Number.isFinite(ms)) return `${left.toFixed(2)},${bottom.toFixed(2)}`;
|
||||
const rawX = ((ms - startMs) / span) * innerW;
|
||||
const x = Math.min(right, Math.max(left, rawX));
|
||||
const rawY = (p.value / maxVal) * innerH;
|
||||
const y = top + Math.max(0, Math.min(innerH, innerH - rawY));
|
||||
return `${x.toFixed(2)},${y.toFixed(2)}`;
|
||||
})
|
||||
.join(' ');
|
||||
}
|
||||
```
|
||||
|
||||
Why this fixes the bug:
|
||||
- `isValidEventWindow` short-circuits the case `endUtc < startUtc` (the persisted repro). The component then renders the existing `Not started`/`No solves yet` empty branch instead of emitting absurd polylines. We do not silently mutate the persisted settings — we just refuse to render an off-canvas chart for an invalid window.
|
||||
- `Math.min/Math.max` clamp surviving X coordinates into `[padL, width - padR]` so future regressions that still produce a malformed span (zero, NaN) cannot escape the SVG viewport.
|
||||
- NaN guards prevent the `toFixed(2)`-on-`NaN` failure mode that previously produced strings like `40.00,292.00`.
|
||||
|
||||
### Step 2 — Component refactor in `score-graph.component.ts`
|
||||
|
||||
- Replace the `@Input() view` with a `signal`-style getter via `ngOnChanges`/`input()` semantics: keep `@Input() view: GraphView | null = null;` (existing pattern) but add a `computed` `seriesWithPoints` that maps each series to `{ ...s, pointsAttr: projectGraphPoints(view, s, dims) }`. Use an explicit `dims: GraphProjectionDims` constant built from the existing `width/height/padL/padR/padT/padB` so the magic numbers are declared once.
|
||||
- Replace the `[attr.points]="pointsAttr(s)"` binding with `[attr.points]="line.pointsAttr"` and iterate over `seriesWithPoints()` instead of `view.series`. (Template keeps `*ngFor` — we are not upgrading to `@for` per job scope.)
|
||||
- Keep all existing `data-testid` markers and the three empty states (`unconfigured` / `countdown` / running-with-empty-series) — the fix is a no-op for those branches.
|
||||
|
||||
### Step 3 — No backend / no DB changes
|
||||
|
||||
The persisted bad data (start > end) stays as-is. The renderer now refuses to project points outside the SVG and gracefully returns empty for the invalid window. We will NOT auto-swap start/end at the backend because that would silently rewrite admin input.
|
||||
|
||||
## 4. Test Strategy
|
||||
|
||||
- **Target Unit Test File:** `tests/frontend/score-graph-projection.spec.ts` (new). It mirrors the existing `tests/frontend/scoreboard.pure.spec.ts` style: plain Jest, no TestBed needed (pure helper), no Angular bootstrap, no DOM rendering.
|
||||
- **Mocking Strategy:**
|
||||
- No mocks required — the helper is a pure function over primitives and date strings.
|
||||
- No HTTP, no time mocking (the helper accepts timestamps as arguments; we pin them in fixtures).
|
||||
- **Tests to include (minimal, focused):**
|
||||
1. `isValidEventWindow` returns `false` when `endUtc < startUtc`, `false` when equal, `false` for non-finite Date strings, `true` only when `end > start`.
|
||||
2. `projectGraphPoints` with the documented repro (`startUtc=2026-07-23T00:00:00.000Z`, `endUtc=2026-07-22T00:00:00.000Z`) returns the empty string — explicitly asserting the bug-class fix.
|
||||
3. `projectGraphPoints` with a normal window returns coordinates where the first point's X is `padL` and the last X is at or below `padL + innerW` (regression guard).
|
||||
4. `projectGraphPoints` with `startUtc === endUtc` (zero-span edge) returns only finite, in-bounds X coordinates (no `Infinity`, no negative).
|
||||
- **Negative coverage deliberately omitted:** we do not mount the Angular component (no TestBed, no fixture) — that keeps the suite fast and matches the rule "Frontend tests focus on logic, not visuals."
|
||||
- **Run command:** `npm test` (executes all projects) or `npm run test:frontend` for the frontend project only. Both commands already work in this repo.
|
||||
|
||||
## Status
|
||||
Not yet implemented. Pure helper `projectGraphPoints` / `isValidEventWindow` do not exist in `scoreboard.pure.ts`; the bug is reproducible from the current `pointsAttr()` method on line 117 of `score-graph.component.ts`.
|
||||
Reference in New Issue
Block a user