Mark/Kimi_Agent_Brother_10_Digitizing_App

Fork 0

paletteIndexForColor returns wrong index for non-adjacent duplicate-color regions #98

New issue

Closed

opened 2026-09-18 02:19:27 +00:00 by Mark · 2 comments

Mark commented

2026-09-18 02:19:27 +00:00

Owner

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

Bug:

paletteIndexForColor is supposed to return the index of color in the
unique-color ordering of regions. The implementation uses seen.size for
the returned index but also calls seen.add(region.color) in the non-matching
branch of the loop, so seen.size ends up being the total number of unique
colors including ones encountered after this color in the iteration, not the
position of this color in the unique sequence.

For regions = [{color: "red"}, {color: "blue"}, {color: "red"}]:

call paletteIndexForColor("red", regions):
- iter 1: red matches. index = seen.size = 0. add red → seen={red}. return 0. ✓
- (would not reach iter 3 on this call)
but regions.map(r => paletteIndexForColor(r.color, regions)):
- r=red: returns 0 (correct).
- r=blue: iter 1 no-match, add red → seen={red}. iter 2 match, index = seen.size = 1. add blue. return 1. ✓
- r=red: iter 1 no-match, add red → seen={red}. iter 2 no-match, add blue → seen={red,blue}. iter 3 match, index = seen.size = 2. add red. return 2. ✗

So the returned array is [0, 1, 2] instead of [0, 1, 0]. A red region that
appears after any non-red region gets the wrong paletteIndex, which then
misaligns it with BROTHER_PR10_COLORS when computing hex codes or matching
against the thread bar.

Repro:

Digitize an image whose color regions include the same color in non-adjacent
positions (e.g. multiple red regions separated by another color).
Open the resulting design — second+ red region will render with the wrong
thread color / wrong position in the BottomThreadBar.

Fix:

Return the count of preceding unique colors, not the full seen.size after
also processing trailing colors. Two small edits:

function paletteIndexForColor(
  color: string,
  regions: ReadonlyArray<{ color: string }>,
): number {
  const seen = new Set<string>();
  for (const region of regions) {
    if (region.color === color) {
      return seen.size;       // count of preceding unique colors
    }
    seen.add(region.color);
  }
  return 0;
}

The inner seen.add(color) is no longer needed (the return runs first).

Acceptance:

New unit test: paletteIndexForColor returns [0, 1, 0] for
[{red}, {blue}, {red}].
New unit test on the reducer or runAutoDigitize end-to-end: digitize a
fixture with non-adjacent duplicate-color regions, assert
colorRegions[i].paletteIndex matches the unique-position, not the
occurrence index.
BottomThreadBar swatch order and hex values match the engine output for the
same fixture.

**Where:** `src/components/workspace/useWorkspace.ts:768-781` (`paletteIndexForColor`). **Bug:** `paletteIndexForColor` is supposed to return the index of `color` in the unique-color ordering of `regions`. The implementation uses `seen.size` for the returned index but also calls `seen.add(region.color)` in the non-matching branch of the loop, so `seen.size` ends up being the total number of unique colors *including ones encountered after this color in the iteration*, not the position of this color in the unique sequence. For `regions = [{color: "red"}, {color: "blue"}, {color: "red"}]`: - call `paletteIndexForColor("red", regions)`: - iter 1: red matches. `index = seen.size = 0`. add red → seen={red}. return 0. ✓ - (would not reach iter 3 on this call) - but `regions.map(r => paletteIndexForColor(r.color, regions))`: - r=red: returns 0 (correct). - r=blue: iter 1 no-match, add red → seen={red}. iter 2 match, `index = seen.size = 1`. add blue. return 1. ✓ - r=red: iter 1 no-match, add red → seen={red}. iter 2 no-match, add blue → seen={red,blue}. iter 3 match, `index = seen.size = 2`. add red. return 2. ✗ So the returned array is `[0, 1, 2]` instead of `[0, 1, 0]`. A red region that appears after any non-red region gets the wrong `paletteIndex`, which then misaligns it with `BROTHER_PR10_COLORS` when computing hex codes or matching against the thread bar. **Repro:** 1. Digitize an image whose color regions include the same color in non-adjacent positions (e.g. multiple red regions separated by another color). 2. Open the resulting design — second+ red region will render with the wrong thread color / wrong position in the BottomThreadBar. **Fix:** Return the count of *preceding* unique colors, not the full `seen.size` after also processing trailing colors. Two small edits: ```ts function paletteIndexForColor( color: string, regions: ReadonlyArray<{ color: string }>, ): number { const seen = new Set<string>(); for (const region of regions) { if (region.color === color) { return seen.size; // count of preceding unique colors } seen.add(region.color); } return 0; } ``` The inner `seen.add(color)` is no longer needed (the `return` runs first). **Acceptance:** - New unit test: `paletteIndexForColor` returns `[0, 1, 0]` for `[{red}, {blue}, {red}]`. - New unit test on the reducer or `runAutoDigitize` end-to-end: digitize a fixture with non-adjacent duplicate-color regions, assert `colorRegions[i].paletteIndex` matches the unique-position, not the occurrence index. - BottomThreadBar swatch order and hex values match the engine output for the same fixture.

Mark added the

bug

triage-approved

labels

2026-09-18 02:19:27 +00:00

Mark commented

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

Author

Owner

False positive — closing.

Repro script (extracted function verbatim, ran with node 22):

function paletteIndexForColor(color, regions) {
  const seen = new Set();
  for (const region of regions) {
    if (region.color === color) {
      const index = seen.size;       // captured BEFORE seen.add
      seen.add(color);
      return index;                  // early return short-circuits the loop
    }
    seen.add(region.color);          // only runs for non-matching preceding regions
  }
  return 0;
}

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

My original trace in the issue body was wrong: the return index inside the
matching branch exits the loop immediately, so seen.add(region.color) in the
non-matching branch only runs for colors that appear before the match, not
after. The function does what it says. Closing as wont-fix / not-a-bug.

Recommend we add a unit test for this function while we're in the area so a
regression of the algorithm can't slip through silently — happy to file that
separately.

False positive — closing. Repro script (extracted function verbatim, ran with node 22): ``` function paletteIndexForColor(color, regions) { const seen = new Set(); for (const region of regions) { if (region.color === color) { const index = seen.size; // captured BEFORE seen.add seen.add(color); return index; // early return short-circuits the loop } seen.add(region.color); // only runs for non-matching preceding regions } return 0; } const r = [{color:'red'},{color:'blue'},{color:'red'}]; console.log(r.map(x => paletteIndexForColor(x.color, r))); // → [0,1,0] (correct) ``` My original trace in the issue body was wrong: the `return index` inside the matching branch exits the loop immediately, so `seen.add(region.color)` in the non-matching branch only runs for colors that appear *before* the match, not *after*. The function does what it says. Closing as wont-fix / not-a-bug. Recommend we add a unit test for this function while we're in the area so a regression of the algorithm can't slip through silently — happy to file that separately.

Mark

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

closed this issue
added
deferred
and removed
bug

triage-approved
labels

Mark referenced this issue from a commit

2026-09-18 02:55:24 +00:00

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

Mark commented

2026-09-18 02:55:30 +00:00

Author

Owner

Adding the regression test you suggested (and a couple of friends) on top of the dead-code tidy:

Exported paletteIndexForColor from useWorkspace.ts (matches the file's existing convention of exporting initialState, derivePESNameFromUpload).
Dropped the dead seen.add(color) after the early return in the matching branch.
Added src/components/workspace/issue-98.test.ts with 7 cases covering: first occurrence, preceding-unique-count, the [red, blue, red] → [0, 1, 0] case from the body, longer non-adjacent sequences ([red, blue, green, red, blue, yellow, green] → [0, 1, 2, 0, 1, 3, 2]), missing color, empty list, and an explicit guard against trailing-color inflation.

Landed as b87c4b3 on main. npm test → 312/312 pass, lint clean, build succeeds. No CI workflow is configured on this repo, so no Actions run to wait on.

Behavior is unchanged from the false-positive fix; this is purely a regression guard.

Adding the regression test you suggested (and a couple of friends) on top of the dead-code tidy: - Exported `paletteIndexForColor` from `useWorkspace.ts` (matches the file's existing convention of exporting `initialState`, `derivePESNameFromUpload`). - Dropped the dead `seen.add(color)` after the early `return` in the matching branch. - Added `src/components/workspace/issue-98.test.ts` with 7 cases covering: first occurrence, preceding-unique-count, the `[red, blue, red]` → `[0, 1, 0]` case from the body, longer non-adjacent sequences (`[red, blue, green, red, blue, yellow, green]` → `[0, 1, 2, 0, 1, 3, 2]`), missing color, empty list, and an explicit guard against trailing-color inflation. Landed as `b87c4b3` on `main`. npm test → 312/312 pass, lint clean, build succeeds. No CI workflow is configured on this repo, so no Actions run to wait on. Behavior is unchanged from the false-positive fix; this is purely a regression guard.

Mark referenced this issue

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

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

Mark referenced this issue

2026-10-02 23:40:02 +00:00

Color Sensitivity slider has no preview — user picks blind, waits ~3s per attempt to see the result #121