src/lib/region-classifier.ts is orphan code - never wired into digitize pipeline (and has a px-vs-mm bug in validateClassification) #88

New issue

Closed

opened 2026-09-14 13:33:15 +00:00 by Mark · 0 comments

Mark commented

2026-09-14 13:33:15 +00:00

Owner

Where: src/lib/region-classifier.ts, src/lib/region-classifier.test.ts.

Bug: This is a TS port of donor easystitch/src/embroidery_converter/classifier.py and easystitch/src/svg_annotation/element_classifier.py that has zero production callers in this repo:

grep -rln 'region-classifier|classifyStitch|@/lib/region-classifier' src/ returns only the test file.
The actual digitize pipeline (src/components/workspace/useWorkspace.ts -> src/lib/easystitch-client.ts -> easystitch-service HTTP /v1/digitize) returns stitchType from the service; the client maps via mapServiceStitchTypeToWorkspace (src/lib/digitize-adapters.ts:158-172). The service does classification server-side.
IMPLEMENTATION_PLAN.md Phase D row #7 ("Decompose: auto-digitize P2 - Region Classification", triage-approved) is the work that was supposed to wire this in. It never landed.

So the module is "Phase 2 work" that exists as tests + a clean port but was never connected. While it is orphan code, two bugs are present in the code path it would take:

Bug 1 - validateClassification compares pixels against mm (lines 329-335).

const { maxDimension } = calculateDimensions(shape.points);
if (stitchType === 'satin' && maxDimension > SATIN_MAX_DIMENSION_MM) {
  warnings.push(`Satin stitch applied to large area (${maxDimension.toFixed(2)}mm). ...`);
}

shape.points are workspace pixels (src/components/workspace/types.ts Point). calculateDimensions returns pixels. SATIN_MAX_DIMENSION_MM = 10.0 is in mm. So at the default MM_TO_PX ~ 3.78 px/mm, ANY satin shape larger than ~2.6 mm in pixels (i.e. larger than ~10 px on screen) triggers a "satin on a large area" warning. In practice this fires on every non-tiny satin shape, so the warning is effectively noise.

The header comment (lines 13-19) explicitly tells callers to convert: const pxThreshold = DEFAULT_WIDTH_THRESHOLD_MM * MM_TO_PX; classifyStitchType(shape, pxThreshold);. validateClassification accepts a widthThreshold parameter (line 326) but never applies it to maxDimension (line 330 void widthThreshold;). Donor _classify_* callers operate in mm throughout (calculate_dimensions(path_data.coordinates) -> mm), so the bug is a porting-only error.

Bug 2 - dead code in classifyStitchSequence (lines 309, 312).

L309: role: shape.fill ? 'original' : 'original' - both arms identical. Either drop the ternary (always 'original') or pick a real 'fill' | 'stroke' | 'original' value based on the shape's actual primary feature.
L312: warnings.length = 0; after a results.push(...) that already used the warnings array. It's clearing a stale local with no effect; only reachable in the non-split branch (the split branch continues before reaching it).

Tests: 30/30 currently pass - but every test feeds points whose pixel-count equals mm-count, so Bug 1 is invisible to the suite. The test fixtures rect(w, h) (line 18) construct points in user-supplied units with no scale, masking the mismatch.

Fix consistent with existing patterns: Three acceptable paths; pick one and the rest follow:

Wire it in (the original Phase D intent):
- Set maxDimension in mm by dividing by MM_TO_PX inside validateClassification (or accept a mmToPx parameter and divide).
- In useWorkspace.ts:runAutoDigitize (post-easystitch-service response), apply classifyStitchSequence to the regions returned for any shape missing a service-assigned stitchType. Honor embroideryType manual override first.
- Replace dead role ternary with a real branch on primaryFeature(shape).
- Drop the dead warnings.length = 0; clear.
- Add a test that constructs a rect in pixels (e.g., 100px x 100px) and asserts no satin warning fires (it would today).
Make it honest (cheaper, matches Path 2 of #83 / #84):
- Delete src/lib/region-classifier.ts and its test file. The actual classifier lives server-side in easystitch-service (pipeline_runner.py).
- Update IMPLEMENTATION_PLAN.md row #7 to deferred with a one-line rationale.
Keep the module, fix the bugs only (cheapest, but leaves it orphan): apply Bug 1 + Bug 2 fixes and leave the integration to Phase D when it actually ships.

Recommended: Path 2. The service has working classification. The TS port is un-wired code with no caller. Tests passing don't justify keeping it. If Phase D ever restarts, the port can be regenerated from the (now stable) donor.

Severity: medium-low. No user-facing bug today (no caller), but the orphan module confuses future contributors about which side owns classification, and the unit bug will bite the moment anyone wires it in.

**Where:** `src/lib/region-classifier.ts`, `src/lib/region-classifier.test.ts`. **Bug:** This is a TS port of donor `easystitch/src/embroidery_converter/classifier.py` and `easystitch/src/svg_annotation/element_classifier.py` that **has zero production callers** in this repo: - `grep -rln 'region-classifier|classifyStitch|@/lib/region-classifier' src/` returns only the test file. - The actual digitize pipeline (`src/components/workspace/useWorkspace.ts` -> `src/lib/easystitch-client.ts` -> easystitch-service HTTP `/v1/digitize`) returns `stitchType` from the service; the client maps via `mapServiceStitchTypeToWorkspace` (`src/lib/digitize-adapters.ts:158-172`). The service does classification server-side. - `IMPLEMENTATION_PLAN.md` Phase D row #7 ("Decompose: auto-digitize P2 - Region Classification", `triage-approved`) is the work that was supposed to wire this in. It never landed. So the module is "Phase 2 work" that exists as tests + a clean port but was never connected. While it is orphan code, two bugs are present in the code path it *would* take: **Bug 1 - `validateClassification` compares pixels against mm (lines 329-335).** ```ts const { maxDimension } = calculateDimensions(shape.points); if (stitchType === 'satin' && maxDimension > SATIN_MAX_DIMENSION_MM) { warnings.push(`Satin stitch applied to large area (${maxDimension.toFixed(2)}mm). ...`); } ``` `shape.points` are workspace pixels (`src/components/workspace/types.ts` `Point`). `calculateDimensions` returns pixels. `SATIN_MAX_DIMENSION_MM = 10.0` is in mm. So at the default `MM_TO_PX ~ 3.78 px/mm`, ANY satin shape larger than ~2.6 mm in pixels (i.e. larger than ~10 px on screen) triggers a "satin on a large area" warning. In practice this fires on every non-tiny satin shape, so the warning is effectively noise. The header comment (lines 13-19) explicitly tells callers to convert: `const pxThreshold = DEFAULT_WIDTH_THRESHOLD_MM * MM_TO_PX; classifyStitchType(shape, pxThreshold);`. `validateClassification` accepts a `widthThreshold` parameter (line 326) but never applies it to `maxDimension` (line 330 `void widthThreshold;`). Donor `_classify_*` callers operate in mm throughout (`calculate_dimensions(path_data.coordinates)` -> mm), so the bug is a porting-only error. **Bug 2 - dead code in `classifyStitchSequence` (lines 309, 312).** - L309: `role: shape.fill ? 'original' : 'original'` - both arms identical. Either drop the ternary (always `'original'`) or pick a real `'fill' | 'stroke' | 'original'` value based on the shape's actual primary feature. - L312: `warnings.length = 0;` after a `results.push(...)` that already used the `warnings` array. It's clearing a stale local with no effect; only reachable in the non-split branch (the split branch `continue`s before reaching it). **Tests:** 30/30 currently pass - but every test feeds points whose pixel-count equals mm-count, so Bug 1 is invisible to the suite. The test fixtures `rect(w, h)` (line 18) construct points in user-supplied units with no scale, masking the mismatch. **Fix consistent with existing patterns:** Three acceptable paths; pick one and the rest follow: 1. **Wire it in** (the original Phase D intent): - Set `maxDimension` in mm by dividing by `MM_TO_PX` inside `validateClassification` (or accept a `mmToPx` parameter and divide). - In `useWorkspace.ts:runAutoDigitize` (post-easystitch-service response), apply `classifyStitchSequence` to the regions returned for any shape missing a service-assigned `stitchType`. Honor `embroideryType` manual override first. - Replace dead `role` ternary with a real branch on `primaryFeature(shape)`. - Drop the dead `warnings.length = 0;` clear. - Add a test that constructs a rect in pixels (e.g., 100px x 100px) and asserts no satin warning fires (it would today). 2. **Make it honest** (cheaper, matches Path 2 of #83 / #84): - Delete `src/lib/region-classifier.ts` and its test file. The actual classifier lives server-side in easystitch-service (`pipeline_runner.py`). - Update `IMPLEMENTATION_PLAN.md` row #7 to `deferred` with a one-line rationale. 3. **Keep the module, fix the bugs only** (cheapest, but leaves it orphan): apply Bug 1 + Bug 2 fixes and leave the integration to Phase D when it actually ships. **Recommended:** Path 2. The service has working classification. The TS port is un-wired code with no caller. Tests passing don't justify keeping it. If Phase D ever restarts, the port can be regenerated from the (now stable) donor. **Severity:** medium-low. No user-facing bug today (no caller), but the orphan module confuses future contributors about which side owns classification, and the unit bug will bite the moment anyone wires it in.