Parcourir la source

fix(inventory): hydrate Extra Colours on edit + render Dual Color as hard-split bars (#1154)

  @maugsburger surfaced two bugs against the original #1154 multi-colour
  swatch work:

  1. Editing an existing spool always opened with the Extra Colours field
     blank, even when the COLOR preview banner above it was rendering
     correctly from the saved data. ColorSection seeded its local
     ``extraColorsDraft`` via ``useState(formData.extra_colors)`` at
     mount time, but SpoolFormModal opens *before* its own useEffect
     populates ``formData`` from the spool record — so by the time the
     saved value landed, the input had already locked onto ''. The user
     then had to retype the value before saving anything else.

  2. Dual Color and Gradient produced the same diagonal blend
     (``linear-gradient(135deg, A, B)``), so the two variants were
     visually indistinguishable. The whole point of the Dual Color variant
     is that the spool has two distinct bars on the reel — a smooth blend
     defeats it.

  Fix:

  - ``ColorSection.tsx``: ref-guarded ``useEffect`` resyncs the draft
    whenever the parent's ``formData.extra_colors`` changes via an
    external update (modal opening with a spool, or switching to a
    different spool mid-form). ``commitExtraColors`` updates the ref
    before calling ``updateField`` so the user's own typing is round-
    tripped without the resync useEffect clobbering it.

  - ``filamentSwatchHelpers.ts``: ``buildColorLayer`` now branches on
    ``effect_type``. ``dual-color`` and ``tri-color`` produce
    ``linear-gradient(to right, c1 0 X%, c2 X% Y%, ...)`` with CSS
    double-position stops — the colour change is a hard vertical line
    rather than a blend region — and equal-width segments across N stops.
    ``gradient`` keeps the original 135° smooth blend. The
    ``multicolor`` conic-gradient path is untouched.

  Tests: 4 new ``FilamentSwatch.test.tsx`` cases pinning the hard-split
  contract (Dual Color uses ``to right`` not ``135deg``; Tri Color
  renders 3 equal hard-split bars; ``gradient`` keeps the smooth
  diagonal; explicit regression guard that Dual Color and Gradient never
  produce the same CSS string for the same stops). 4 new
  ``ColorSectionExtraColorsHydration.test.tsx`` cases pinning the input
  hydration (fills when formData arrives via parent update, resyncs when
  the spool changes mid-form, doesn't clobber live user typing, clears
  when the new spool has no extra_colors). Full frontend suite: 1608
  passed; full backend suite: 3598 passed; no regressions.

  The minor "Sparkle could be more prominent / checkerboard denser"
  feedback in the same comment is deferred to a separate cosmetic pass —
  the reporter flagged it as finetuning.
maziggy il y a 3 semaines
Parent
commit
6a1e3454db

Fichier diff supprimé car celui-ci est trop grand
+ 0 - 0
CHANGELOG.md


+ 73 - 0
frontend/src/__tests__/components/FilamentSwatch.test.tsx

@@ -118,6 +118,79 @@ describe('FilamentSwatch', () => {
   });
 });
 
+describe('dual-color / tri-color hard-split bars (#1154 follow-up)', () => {
+  // Bug: the original #1154 fix produced an identical
+  // ``linear-gradient(135deg, A, B)`` for both Gradient and Dual Color
+  // effects, so a "Dual Color" spool looked indistinguishable from a
+  // "Gradient" one — both rendered as a smooth diagonal blend. Real
+  // dual-colour spools have two visually distinct bars, not a blend.
+  // These tests pin the corrected rendering: a horizontal hard split
+  // for dual-color / tri-color, the original 135° smooth blend for
+  // everything else.
+
+  it('renders dual-color as a hard horizontal split, not a diagonal blend', () => {
+    const bg = buildFilamentBackground({
+      extraColors: '7f3696,006ec9',
+      effectType: 'dual-color',
+    });
+    const lower = bg.toLowerCase();
+    // Hard split direction — ``to right`` (or ``90deg``), never ``135deg``.
+    expect(lower).toContain('to right');
+    expect(lower).not.toContain('135deg');
+    // Both colour stops present.
+    expect(lower).toContain('#7f3696');
+    expect(lower).toContain('#006ec9');
+    // Each colour occupies its own segment via double-position stops, so
+    // the colour change is a hard line rather than a blend region.
+    expect(lower).toMatch(/#7f3696\s+0\.000%\s+50\.000%/);
+    expect(lower).toMatch(/#006ec9\s+50\.000%\s+100\.000%/);
+  });
+
+  it('renders tri-color as three equal hard-split bars', () => {
+    const bg = buildFilamentBackground({
+      extraColors: 'ff0000,00ff00,0000ff',
+      effectType: 'tri-color',
+    });
+    const lower = bg.toLowerCase();
+    expect(lower).toContain('to right');
+    // Each third gets its own contiguous segment.
+    expect(lower).toMatch(/#ff0000\s+0\.000%\s+33\.333%/);
+    expect(lower).toMatch(/#00ff00\s+33\.333%\s+66\.667%/);
+    expect(lower).toMatch(/#0000ff\s+66\.667%\s+100\.000%/);
+  });
+
+  it('keeps the smooth 135° diagonal for the default Gradient effect', () => {
+    const bg = buildFilamentBackground({
+      extraColors: '7f3696,006ec9',
+      effectType: 'gradient',
+    });
+    const lower = bg.toLowerCase();
+    // Original visual preserved for non-dual / non-tri stops.
+    expect(lower).toContain('135deg');
+    expect(lower).not.toContain('to right');
+    // Stops are concatenated without explicit positions — CSS does the
+    // smooth blend across the diagonal.
+    expect(lower).toContain('#7f3696');
+    expect(lower).toContain('#006ec9');
+  });
+
+  it('regression: dual-color and gradient produce visually distinct backgrounds', () => {
+    // Direct regression guard for the reporter's exact symptom — the two
+    // effects must NOT collapse to the same CSS string. If a future refactor
+    // accidentally drops the dual-color branch, this assertion fires before
+    // anyone has to retest in a browser.
+    const dual = buildFilamentBackground({
+      extraColors: '7f3696,006ec9',
+      effectType: 'dual-color',
+    });
+    const grad = buildFilamentBackground({
+      extraColors: '7f3696,006ec9',
+      effectType: 'gradient',
+    });
+    expect(dual).not.toBe(grad);
+  });
+});
+
 describe('buildFilamentBackground', () => {
   it('emits the same layered background string the component renders', () => {
     const bg = buildFilamentBackground({

+ 139 - 0
frontend/src/__tests__/components/spool-form/ColorSectionExtraColorsHydration.test.tsx

@@ -0,0 +1,139 @@
+/**
+ * Regression tests for ColorSection extra_colors hydration (#1154 follow-up).
+ *
+ * Bug: when the SpoolFormModal opens to edit an existing spool, the parent
+ * component renders ColorSection IMMEDIATELY (with default-empty formData),
+ * then fills formData via a useEffect a tick later. The previous
+ * implementation seeded ``extraColorsDraft`` from formData via
+ * ``useState(formData.extra_colors)`` only at mount time, so the late
+ * arrival of the saved value never made it into the input — the field
+ * appeared blank even though the COLOR preview banner above was rendering
+ * correctly from the now-populated formData.
+ *
+ * Fix: a ref-guarded useEffect resyncs the draft when the parent's
+ * formData.extra_colors changes via an external update (e.g. modal opening
+ * with a spool). User typing routes through ``commitExtraColors`` which
+ * updates the ref before calling ``updateField``, so the resync useEffect
+ * sees a no-op on round-tripped values and doesn't clobber the user's text.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import { render, screen, fireEvent } from '@testing-library/react';
+import { I18nextProvider } from 'react-i18next';
+import i18n from '../../../i18n';
+import { ColorSection } from '../../../components/spool-form/ColorSection';
+import { defaultFormData } from '../../../components/spool-form/types';
+
+type FormData = typeof defaultFormData;
+
+function renderWithFormData(initial: Partial<FormData>) {
+  const formData = { ...defaultFormData, ...initial };
+  const updateField = vi.fn();
+
+  const { rerender } = render(
+    <I18nextProvider i18n={i18n}>
+      <ColorSection
+        formData={formData}
+        updateField={updateField}
+        recentColors={[]}
+        onColorUsed={vi.fn()}
+        catalogColors={[]}
+      />
+    </I18nextProvider>,
+  );
+
+  return {
+    updateField,
+    /**
+     * Re-render with a different formData object — simulates the parent's
+     * useEffect-driven setFormData(...) that fills the form from the spool
+     * record after the modal mounts.
+     */
+    updateFormData: (next: Partial<FormData>) => {
+      const merged = { ...defaultFormData, ...initial, ...next };
+      rerender(
+        <I18nextProvider i18n={i18n}>
+          <ColorSection
+            formData={merged}
+            updateField={updateField}
+            recentColors={[]}
+            onColorUsed={vi.fn()}
+            catalogColors={[]}
+          />
+        </I18nextProvider>,
+      );
+    },
+  };
+}
+
+function findExtraColorsInput(): HTMLInputElement {
+  // The input is identified by its English placeholder copy. If the i18n
+  // string changes we'll need to update this — preferred over a brittle
+  // ``getByDisplayValue`` because the value is the thing under test.
+  return screen.getByPlaceholderText(/EC984C,/i) as HTMLInputElement;
+}
+
+describe('ColorSection extra_colors hydration (#1154 follow-up)', () => {
+  it('hydrates the input when formData.extra_colors arrives via a parent update', () => {
+    // Mount with empty formData (the realistic state when SpoolFormModal
+    // opens — it conditionally renders the modal before its own effect
+    // fills formData from the spool).
+    const { updateFormData } = renderWithFormData({ extra_colors: '' });
+
+    const input = findExtraColorsInput();
+    expect(input.value).toBe('');
+
+    // Parent's setFormData lands a tick later with the spool's saved
+    // extra_colors. The component must pick up the new value.
+    updateFormData({ extra_colors: 'ec984c,6cd4bc,a66eb9,d87694' });
+
+    const refreshedInput = findExtraColorsInput();
+    expect(refreshedInput.value).toBe('ec984c,6cd4bc,a66eb9,d87694');
+  });
+
+  it('resyncs when the spool changes (e.g. user edits a different spool)', () => {
+    // Mount already-populated, then swap to a different spool's data —
+    // the field should track the new spool, not stick on the first value.
+    const { updateFormData } = renderWithFormData({
+      extra_colors: 'aabbcc,ddeeff',
+    });
+    expect(findExtraColorsInput().value).toBe('aabbcc,ddeeff');
+
+    updateFormData({ extra_colors: '7f3696,006ec9' });
+    expect(findExtraColorsInput().value).toBe('7f3696,006ec9');
+  });
+
+  it('does not clobber user typing on the same render cycle', () => {
+    // The fix uses a ref to track our own commits so the resync effect
+    // doesn't fight the user. Type into the field, trigger a parent
+    // re-render (no formData change), and verify the typed value sticks.
+    const { updateFormData, updateField } = renderWithFormData({
+      extra_colors: '',
+    });
+
+    const input = findExtraColorsInput();
+    fireEvent.change(input, { target: { value: 'ff0000,00ff00' } });
+
+    // The commit handler updates the canonical formData via updateField.
+    // Simulate the parent reflecting that change back into formData on
+    // the next render — the input must keep showing the user's text, not
+    // collapse to '' (the previous round-trip bug shape).
+    expect(updateField).toHaveBeenCalledWith('extra_colors', 'ff0000,00ff00');
+    updateFormData({ extra_colors: 'ff0000,00ff00' });
+
+    expect(findExtraColorsInput().value).toBe('ff0000,00ff00');
+  });
+
+  it('clears the input when the spool has no extra_colors', () => {
+    // The previous-spool-with-colours → next-spool-without-colours flow
+    // must clear the field; otherwise opening a plain solid spool right
+    // after a multi-colour one would show stale text.
+    const { updateFormData } = renderWithFormData({
+      extra_colors: 'aabbcc,ddeeff',
+    });
+    expect(findExtraColorsInput().value).toBe('aabbcc,ddeeff');
+
+    updateFormData({ extra_colors: '' });
+    expect(findExtraColorsInput().value).toBe('');
+  });
+});

+ 28 - 6
frontend/src/components/filamentSwatchHelpers.ts

@@ -128,8 +128,16 @@ export function parseStops(extra: string | null | undefined): string[] {
 }
 
 /** Build the colour layer (gradient or solid) given rgba + stops + subtype/effect.
- *  A conic gradient is used when either subtype OR effect_type is `Multicolor`,
- *  giving the catalog editor a way to flag a multicolor variant directly. */
+ *  - ``multicolor`` (subtype OR effect): conic gradient — the swatch reads as
+ *    a colour wheel pie, distinct from a stripe.
+ *  - ``dual-color`` / ``tri-color`` (effect): hard-split horizontal bars with
+ *    no diagonal blend. A 2-stop Dual Color renders as left/right halves of
+ *    distinct colour, matching the way real dual-colour spools look on the
+ *    reel — without this, Gradient and Dual Color produced the same diagonal
+ *    blend and were visually indistinguishable (#1154 follow-up).
+ *  - everything else (``gradient`` and the default): a smooth 135° linear
+ *    gradient across the stops, the original visual for blended-stop spools.
+ */
 export function buildColorLayer(
   rgba: string | null | undefined,
   stops: string[],
@@ -143,12 +151,26 @@ export function buildColorLayer(
   }
   // With stops we ignore the single rgba and gradient across the stops.
   const allStops = stops.length === 1 ? [stops[0], stops[0]] : stops;
-  const isMulticolor =
-    (subtype ?? '').toLowerCase() === 'multicolor' ||
-    (effectType ?? '').toLowerCase() === 'multicolor';
-  if (isMulticolor) {
+  const subtypeLower = (subtype ?? '').toLowerCase();
+  const effectLower = (effectType ?? '').toLowerCase();
+  if (subtypeLower === 'multicolor' || effectLower === 'multicolor') {
     return `conic-gradient(from 0deg, ${allStops.join(', ')}, ${allStops[0]})`;
   }
+  if (effectLower === 'dual-color' || effectLower === 'tri-color') {
+    // Equal-width hard-split bars: each stop occupies its own contiguous
+    // segment with no blend across the boundary. CSS double-position stops
+    // (``c X% Y%``) collapse the transition zone to zero so the colour
+    // change is a hard vertical line rather than a diagonal smear.
+    const n = allStops.length;
+    const segments = allStops
+      .map((c, i) => {
+        const start = ((i / n) * 100).toFixed(3);
+        const end = (((i + 1) / n) * 100).toFixed(3);
+        return `${c} ${start}% ${end}%`;
+      })
+      .join(', ');
+    return `linear-gradient(to right, ${segments})`;
+  }
   return `linear-gradient(135deg, ${allStops.join(', ')})`;
 }
 

+ 19 - 1
frontend/src/components/spool-form/ColorSection.tsx

@@ -1,4 +1,4 @@
-import { useState, useMemo } from 'react';
+import { useState, useMemo, useEffect, useRef } from 'react';
 import { Search, Clock, ChevronDown, ChevronUp, Sparkles } from 'lucide-react';
 import { useTranslation } from 'react-i18next';
 import type { ColorSectionProps, CatalogDisplayColor } from './types';
@@ -170,6 +170,22 @@ export function ColorSection({
   // commit the canonical form to formData on blur or when valid.
   const [extraColorsDraft, setExtraColorsDraft] = useState<string>(formData.extra_colors);
   const [extraColorsErrors, setExtraColorsErrors] = useState<string[]>([]);
+
+  // #1154 follow-up: when the modal opens to edit an existing spool, the
+  // parent's ``setFormData(...)`` lands in a useEffect AFTER ColorSection
+  // already mounted with the default-empty formData. Without resyncing,
+  // ``extraColorsDraft`` stays at the initial '' and the field appears
+  // empty even though the spool has saved colours (visible as the gradient
+  // banner above). Track our own commits via a ref so external formData
+  // updates resync the draft without clobbering live user typing.
+  const lastCommittedExtraColorsRef = useRef<string>(formData.extra_colors);
+  useEffect(() => {
+    if (formData.extra_colors !== lastCommittedExtraColorsRef.current) {
+      setExtraColorsDraft(formData.extra_colors);
+      setExtraColorsErrors([]);
+      lastCommittedExtraColorsRef.current = formData.extra_colors;
+    }
+  }, [formData.extra_colors]);
   const previewBackground = useMemo(
     () =>
       buildFilamentBackground({
@@ -185,11 +201,13 @@ export function ColorSection({
     setExtraColorsDraft(text);
     if (!text.trim()) {
       setExtraColorsErrors([]);
+      lastCommittedExtraColorsRef.current = '';
       updateField('extra_colors', '');
       return;
     }
     const { value, invalid } = normalizeExtraColorsInput(text);
     setExtraColorsErrors(invalid);
+    lastCommittedExtraColorsRef.current = value;
     updateField('extra_colors', value);
   };
 

Fichier diff supprimé car celui-ci est trop grand
+ 0 - 0
static/assets/index-DUxwTpts.js


+ 1 - 1
static/index.html

@@ -26,7 +26,7 @@
 
     <!-- Splash screens for iOS -->
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
-    <script type="module" crossorigin src="/assets/index-BlyoHMPZ.js"></script>
+    <script type="module" crossorigin src="/assets/index-DUxwTpts.js"></script>
     <link rel="stylesheet" crossorigin href="/assets/index-7GmlJb0k.css">
   </head>
   <body>

Certains fichiers n'ont pas été affichés car il y a eu trop de fichiers modifiés dans ce diff