Add unit tests for paletteIndexForColor — zero coverage on a non-trivial algorithm #99

New issue

Closed

opened 2026-09-18 02:38:39 +00:00 by Mark · 1 comment

Mark commented

2026-09-18 02:38:39 +00:00

Owner

Where: src/components/workspace/useWorkspace.ts:768-781 (paletteIndexForColor).

Why:

paletteIndexForColor is a private helper used at useWorkspace.ts:603 to
assign paletteIndex values to incoming ColorRegions. The function is
non-trivial (early-return inside a loop, seen.add in both branches) and
is called with whatever response.regions shape the easystitch-service
returns. Today it has zero direct tests.

I proposed a (false-positive) bug against it earlier this session that turned
out to be wrong, but the exercise showed the lack of unit coverage is the
real problem — without a test, both false positives AND genuine regressions
slip through. Recommend adding coverage so the algorithm can't drift.

Test cases the existing function passes (verified by node 22 inline repro
extracted from source):

const r = [{color:'red'},{color:'blue'},{color:'red'}];
r.map(x => paletteIndexForColor(x.color, r)) // → [0,1,0]

const u = [{color:'red'},{color:'blue'},{color:'green'}];
u.map(x => paletteIndexForColor(x.color, u)) // → [0,1,2]

const e = [{color:'red'},{color:'blue'},{color:'red'},{color:'green'},{color:'red'}];
e.map(x => paletteIndexForColor(x.color, e)) // → [0,1,0,2,0]

The function relies on early-return inside the matching branch to keep
seen.add(region.color) from polluting the count with post-match colors.
That's an easy thing to break in a refactor (e.g. moving seen.add(color)
outside the if, or removing the return to fall through). Without tests,
this breaks silently and the symptom is misaligned thread palette in the
BottomThreadBar / export.

Suggested fix:

Export paletteIndexForColor from useWorkspace.ts (or move it to
src/lib/palette.ts as a pure helper and import it back) and add
src/lib/palette.test.ts covering:

Already-unique regions → identity [0..N-1].
Two reds separated by blue → [0,1,0].
Three reds separated by alternating other colors → [0,1,0,2,0].
Empty regions list → all calls return 0.
Asking for a color not present → returns 0.
End-to-end through the reducer: dispatch SET_COLOR_REGIONS with
duplicated-color regions, assert resulting colorRegions[i].paletteIndex
matches the unique-position (not the occurrence index).

Acceptance:

Test file at src/lib/palette.test.ts (or alongside the helper if moved).
6 tests covering the cases above all pass.
npm test shows the new file in the run summary.

**Where:** `src/components/workspace/useWorkspace.ts:768-781` (`paletteIndexForColor`). **Why:** `paletteIndexForColor` is a private helper used at `useWorkspace.ts:603` to assign `paletteIndex` values to incoming `ColorRegion`s. The function is non-trivial (early-return inside a loop, `seen.add` in both branches) and is called with whatever `response.regions` shape the easystitch-service returns. Today it has zero direct tests. I proposed a (false-positive) bug against it earlier this session that turned out to be wrong, but the exercise showed the lack of unit coverage is the real problem — without a test, both false positives AND genuine regressions slip through. Recommend adding coverage so the algorithm can't drift. **Test cases the existing function passes (verified by node 22 inline repro extracted from source):** ``` const r = [{color:'red'},{color:'blue'},{color:'red'}]; r.map(x => paletteIndexForColor(x.color, r)) // → [0,1,0] const u = [{color:'red'},{color:'blue'},{color:'green'}]; u.map(x => paletteIndexForColor(x.color, u)) // → [0,1,2] const e = [{color:'red'},{color:'blue'},{color:'red'},{color:'green'},{color:'red'}]; e.map(x => paletteIndexForColor(x.color, e)) // → [0,1,0,2,0] ``` The function relies on early-return inside the matching branch to keep `seen.add(region.color)` from polluting the count with post-match colors. That's an easy thing to break in a refactor (e.g. moving `seen.add(color)` outside the `if`, or removing the `return` to fall through). Without tests, this breaks silently and the symptom is misaligned thread palette in the BottomThreadBar / export. **Suggested fix:** Export `paletteIndexForColor` from `useWorkspace.ts` (or move it to `src/lib/palette.ts` as a pure helper and import it back) and add `src/lib/palette.test.ts` covering: 1. Already-unique regions → identity `[0..N-1]`. 2. Two reds separated by blue → `[0,1,0]`. 3. Three reds separated by alternating other colors → `[0,1,0,2,0]`. 4. Empty regions list → all calls return `0`. 5. Asking for a color not present → returns `0`. 6. End-to-end through the reducer: dispatch `SET_COLOR_REGIONS` with duplicated-color regions, assert resulting `colorRegions[i].paletteIndex` matches the unique-position (not the occurrence index). **Acceptance:** - Test file at `src/lib/palette.test.ts` (or alongside the helper if moved). - 6 tests covering the cases above all pass. - `npm test` shows the new file in the run summary.

Mark added the

security/medium

feature-request

labels

2026-09-18 02:38:39 +00:00

Mark added the

triage-approved

label

2026-09-18 05:05:46 +00:00

Mark commented

2026-09-18 05:12:39 +00:00

Author

Owner

Closing as duplicate — the recommendation in this issue was implemented under #98:

b87c4b3 test(#98): lock in paletteIndexForColor behavior; tidy dead seen.add in match branch

That commit already shipped on main:

Exported paletteIndexForColor from useWorkspace.ts (so it can be unit-tested directly).
Added src/components/workspace/issue-98.test.ts with 7 cases covering all 6 acceptance criteria from this issue (already-unique regions, non-adjacent duplicates, longer alternating sequences, empty list, missing color, end-to-end through the reducer).
Tidied the dead seen.add(color) after the early return.
Verified: 7/7 vitest pass, npm test 312/312, lint clean, build green.

Per #98 comment #56141 the original false-positive was also confirmed against the shipped code. This issue is a follow-up recommendation to that work and the recommendation is fulfilled.

Closing as duplicate — the recommendation in this issue was implemented under #98: b87c4b3 test(#98): lock in paletteIndexForColor behavior; tidy dead seen.add in match branch That commit already shipped on `main`: - Exported `paletteIndexForColor` from `useWorkspace.ts` (so it can be unit-tested directly). - Added `src/components/workspace/issue-98.test.ts` with 7 cases covering all 6 acceptance criteria from this issue (already-unique regions, non-adjacent duplicates, longer alternating sequences, empty list, missing color, end-to-end through the reducer). - Tidied the dead `seen.add(color)` after the early return. - Verified: 7/7 vitest pass, npm test 312/312, lint clean, build green. Per #98 comment #56141 the original false-positive was also confirmed against the shipped code. This issue is a follow-up recommendation to that work and the recommendation is fulfilled.

Mark

2026-09-18 05:12:39 +00:00