fix(spoolman): persist Color Name via spool.extra — Spoolman has no filament.color_name field (#1357)

Reporter pgladel edited a spool's Color Name in Spoolman mode, hit Save, and saw the value snap back to the subtype on the next read. The earlier #1319 fix correctly handled the read/form-prefill half (the color_name_is_synthesized flag, blank-on-synth form init), but the write half assumed Spoolman has a `color_name` field on Filament. It doesn't. Verified against the live FilamentUpdateParameters schema on Spoolman 0.23.1 — the accepted fields are name, vendor_id, material, price, density, diameter, weight, spool_weight, article_number, comment, settings_extruder_temp, settings_bed_temp, color_hex, multi_color_hexes, multi_color_direction, external_id, extra. No color_name. Spoolman's PATCH happily returns 200 for {"color_name": "Red"} and silently discards the unknown key, so find_or_create_filament was either patching a void or creating filament after filament with the same field-that-doesn't-stick (which is what produced the "BB also created a bunch of new filaments" duplicate trail on each save attempt). The fix follows the same pattern as the existing BambuStudio slicer- preset storage: persist color_name on spool.extra.bambu_color_name as a JSON-encoded string, register the extra field via ensure_extra_field before write (Spoolman 400s on unknown extra keys), and read it back in _map_spoolman_spool with priority extra > filament.color_name (forward-compat for any future Spoolman release that adds the field) > subtype synth. Dropped the now-dead color_name passing through find_or_create_filament and create_filament — Spoolman would discard it anyway and keeping the dead pipe risked the same confusion the next time someone reads this code. The previous "match by name then patch color_name" loop is gone; what survives is the name-match resilience that lets an AMS-sync-created filament named "Glow" still match the user-driven edit's composed "PLA Glow", which prevents re-introducing the duplicate-filament trail. The frontend form's color_name_is_synthesized handling is unchanged — that part already worked.
2026-10-09 15:35:39 +02:00 · 2026-05-17 09:10:20 +02:00
parent 135b8fd93b
commit 4a98914d4a
7 changed files with 301 additions and 129 deletions
@@ -5,6 +5,7 @@ All notable changes to Bambuddy will be documented in this file.
 ## [0.2.5b1] - Unreleased

 ### Fixed
+- **Spoolman: spool "Color Name" edits silently never saved — Bambuddy was writing to a field Spoolman doesn't have (#1357)** — Reporter pgladel edited a spool's Color Name in Spoolman mode, hit Save, and saw the value snap back to the subtype on the next read. Martin shipped #1319 in May to handle "form round-trips the synth value back as if it were user input" — that fix's read/form-prefill half was correct (the `color_name_is_synthesized` flag, the blank-on-synth form init), but the **write half assumed Spoolman has a `color_name` field on Filament**. It doesn't. Verified against the live `FilamentUpdateParameters` schema on Spoolman 0.23.1: `name`, `vendor_id`, `material`, `price`, `density`, `diameter`, `weight`, `spool_weight`, `article_number`, `comment`, `settings_extruder_temp`, `settings_bed_temp`, `color_hex`, `multi_color_hexes`, `multi_color_direction`, `external_id`, `extra` — that's the lot. No `color_name`. Spoolman's PATCH happily returns 200 for `{"color_name": "Red"}` and just **silently discards the unknown key**. So `find_or_create_filament` was either patching a void or creating filament after filament with the same field-that-doesn't-stick (which is what produced the reporter's "BB also created a bunch of new filaments" trail of duplicates on each save attempt). The fix takes the same route as the existing BambuStudio slicer-preset storage: persist color_name on `spool.extra.bambu_color_name` as a JSON-encoded string, register the extra field via `ensure_extra_field` before write (Spoolman 400s on unknown extra keys), and read it back in `_map_spoolman_spool` with priority `spool.extra.bambu_color_name → filament.color_name (forward-compat for any future Spoolman release that adds it) → subtype synth`. Also dropped the now-dead `color_name` passing through `find_or_create_filament` and `create_filament` — Spoolman would discard it anyway and keeping the dead pipe risked the same confusion the next time someone reads this code. The previous "match by name then patch color_name" loop is gone; what survives is the name-match resilience added earlier this turn so an AMS-sync-created filament named `"Glow"` still matches the user-driven edit's composed `"PLA Glow"`, which prevents the duplicate-filament trail. The frontend form's `color_name_is_synthesized` handling is unchanged — that part already worked. Tests rewritten across the three affected suites (`test_spoolman_inventory_methods.py`, `test_spoolman_inventory_helpers.py`, `test_spoolman_inventory_api.py`) to pin the new contract: filament patch never carries `color_name`, route writes to `bambu_color_name` extra, read prefers extra over filament-field over synth. Verified end-to-end against the live Spoolman instance at the reporter's setup (PATCH /filament with color_name → field absent from response; PATCH /spool with extra.bambu_color_name → field present in response).
 - **Add Smart Plug (HA mode) — search dropdown let users pick entities the schema would reject, surfacing as a cryptic regex error on Save (#1388)** — Reporter MartinNYHC opened the Add Smart Plug dialog, typed a search prefix matching a multi-entity HA device (a Shelly-style outlet exposing one `switch.*` and several `sensor.*` / `binary_sensor.*` siblings under the same friendly-name prefix), clicked one of the entities, filled in the optional power/energy sensors, and clicked Save. The backend returned 422 with the raw Pydantic message `String should match pattern '^(switch|light|input_boolean|script)\.[a-z0-9_]+$'`. After the dropdown closed and the search cleared, the entity-list refetch (with no search param) returned the default-domain-filtered list — which didn't include the user's pick — so `selectedEntity = haEntities.find(...)` was undefined, the field rendered as visually empty (placeholder shown), but `haEntityId` still held the bad value the user had selected. Root cause was at `backend/app/services/homeassistant.py::list_entities`: when a search query was present, the function bypassed the domain filter entirely and returned matches across every HA domain — including ones the `SmartPlugBase.ha_entity_id` regex at `backend/app/schemas/smart_plug.py:17` could never accept. Offering a clickable choice the user can't save is broken UX; the fact that the error message then said `switch|light|input_boolean|script` made it look like a schema problem rather than a search-permissiveness problem. Fix: the allowed-domains filter (`{"switch", "light", "input_boolean", "script"}`, kept in sync with the schema regex) now always runs, and search composes on top of it as an additional substring match against `entity_id` or `friendly_name`. Whitespace-only search strings are treated as no search. Verified the smart-plug code path is unchanged between 0.2.4 and 0.2.4.1 — this bug was latent since the script-domain commit in February 2026 and was only noticed now because the reporter hadn't reopened the modal in months. 5 new regression tests in `backend/tests/unit/services/test_homeassistant_list_entities.py` cover the no-search baseline, the search-still-domain-filters case (the actual #1388 reproduction), the entity_id-or-friendly_name substring match, case-insensitivity, and the whitespace-only edge case.
 - **H2S with no AMS could not start a print — firmware rejected the dispatch with `07FF_8012` "Failed to get AMS mapping table" (#1386)** — Reporter krootstijn (H2S + no AMS) clicked Print and got an immediate firmware error. Two stacked misclassifications had quietly added H2S to the dual-nozzle code paths over time. The first was in `start_print_job` at `backend/app/services/bambu_mqtt.py:3168` — the `is_h2d` flag was set true for `("H2D", "H2D PRO", "H2DPRO", "H2C", "H2S", "X2D")`. That single flag controlled both the firmware bool→int format (legitimately needed for the whole H-family) *and* the external-spool routing branch (`ext_ams_id = tray_id if is_h2d else 255`) which is only correct for actual dual-nozzle printers. With no AMS, the external-spool sentinel is `254`; the dual-nozzle branch wrote `ams_id=254` into `ams_mapping2` instead of the canonical `255`. The exact failure shape (`07FF_8012`) is even called out in the comment six lines above the bad line — H2S was getting routed straight into the path the comment warned against. The second misclassification was the use_ams=False fallback at `bambu_mqtt.py:3213` (`if ams_mapping and use_ams and not is_h2d`) — meant to skip the safety drop on dual-nozzle printers where `use_ams` controls nozzle routing — also skipped H2S, so the firmware never got a chance to fall back to external-spool mode. A third site at `bambu_mqtt.py:3987` (and its sibling at `backend/app/api/routes/kprofiles.py:119`) classified dual-nozzle by serial prefix `("094", "20P9", "31B8B")`, which is wrong because H2S shares prefix `094` with H2D. Fix splits the conflated flag into two: `is_h_family` (firmware-format gate, includes H2S) and `is_dual_nozzle` (routing/use_ams gate, excludes H2S; prefers the runtime `_is_dual_nozzle` flag set from `device.extruder.info` and falls back to model name for the brief window right after connect). The K-profile delete and the edit route now use the same two-source check instead of the serial prefix. Empirically verified across 9+ stored H2S support bundles (`nozzle_count: 1` in every one) and the reporter's bug log (`07FF_8012` immediately after dispatch). Four new regression tests: `test_h2s_single_external_spool_uses_main_id`, `test_h2s_no_ams_forces_use_ams_false`, `test_h2s_keeps_integer_format_for_calibration_fields`, plus a new `test_h2s_uses_single_nozzle_format` in the K-profile suite. The K-profile detection tests were also updated to set both model name and runtime flag rather than relying on serial prefix, since the source-of-truth has shifted.

@@ -257,18 +257,28 @@ def _map_spoolman_spool(spool: dict) -> MappedSpoolFields:

    created_at: str | None = spool.get("registered") or None

-    # Spoolman doesn't standardise a `color_name` field — most installs only
-    # populate `color_hex` (the swatch) and the filament's `name` (which often
-    # carries the colour, e.g. "PLA Basic Red"). Without a fallback the
-    # frontend lists a sea of "Unknown color" entries that all look identical
-    # except for the swatch. Fall back to the filament name minus material
-    # prefix (the same string the `subtype` field already carries — typically
-    # "Basic Red" / "PLA+ Black" / etc.) so the user can tell spools apart at
-    # a glance even on Spoolman installs that don't fill color_name.
-    # color_name_is_synthesized: surfaced so the edit form can avoid prefilling
-    # the synth value back into the input, which would otherwise round-trip the
-    # subtype string as if it were a user-set color_name (#1319).
-    stored_color_name = filament.get("color_name") or None
+    # Spoolman has no `color_name` field on Filament — confirmed against the
+    # FilamentUpdateParameters schema in 0.23.1: name/vendor_id/material/price/
+    # density/diameter/weight/spool_weight/article_number/comment/extruder_temp/
+    # bed_temp/color_hex/multi_color_hexes/multi_color_direction/external_id/
+    # extra, no color_name (#1357). The previous attempt (b8e350c3) was
+    # PATCHing a key Spoolman silently discards, which is why color_name
+    # never actually persisted from the user's edits.
+    #
+    # We persist it ourselves under spool.extra.bambu_color_name (JSON-encoded
+    # string, same pattern as bambu_slicer_filament). Read order:
+    #   1. spool.extra.bambu_color_name (the canonical store)
+    #   2. filament.color_name (forward-compat — picks up the value if a
+    #      future Spoolman release adds the field, or if an admin populated
+    #      it via a custom extra-field they registered themselves)
+    #   3. subtype (synth fallback so the inventory list isn't a sea of
+    #      "Unknown color" entries on installs with neither field set)
+    #
+    # color_name_is_synthesized = True only when we fell back to subtype.
+    # The edit form uses it to leave the input blank, so the user doesn't
+    # round-trip the synth value back as if they had set it.
+    extra_color_name = _extract_extra_str(extra, "bambu_color_name") or None
+    stored_color_name = extra_color_name or (filament.get("color_name") or None)
    color_name: str | None = stored_color_name or subtype or None
    color_name_is_synthesized: bool = stored_color_name is None and color_name is not None

@@ -440,18 +440,24 @@ async def create_spool(

    spool, price_warnings = await _apply_price_if_set(client, spool, data.cost_per_kg)

-    # Persist slicer_filament under the spool's extra dict (mirror update_spool).
-    if data.slicer_filament is not None or data.slicer_filament_name is not None:
+    # Persist slicer_filament AND color_name under the spool's extra dict
+    # (mirror update_spool). Spoolman has no `color_name` field on filament
+    # (#1357) so we own the round-trip ourselves.
+    if data.slicer_filament is not None or data.slicer_filament_name is not None or data.color_name is not None:
        # Ensure extra fields are registered before write.
        if data.slicer_filament is not None:
            await client.ensure_extra_field("bambu_slicer_filament")
        if data.slicer_filament_name is not None:
            await client.ensure_extra_field("bambu_slicer_filament_name")
+        if data.color_name is not None:
+            await client.ensure_extra_field("bambu_color_name")
        new_extra: dict = {}
        if data.slicer_filament is not None:
            new_extra["bambu_slicer_filament"] = json.dumps(data.slicer_filament)
        if data.slicer_filament_name is not None:
            new_extra["bambu_slicer_filament_name"] = json.dumps(data.slicer_filament_name)
+        if data.color_name is not None:
+            new_extra["bambu_color_name"] = json.dumps(data.color_name)
        if new_extra:
            try:
                async with _translate_spoolman_errors():
@@ -459,7 +465,7 @@ async def create_spool(
            except HTTPException:
                # Best-effort — the spool already exists, log and continue.
                logger.warning(
-                    "Failed to persist slicer_filament for spool %s",
+                    "Failed to persist slicer_filament/color_name for spool %s",
                    spool.get("id"),
                )

@@ -633,25 +639,33 @@ async def update_spool(
                clear_location=storage_location_changed and not storage_location,
            )

-    # Persist BambuStudio slicer preset under the spool's extra dict.
-    # Spoolman doesn't have a native field for this, so we round-trip via
-    # extra and unpack in _map_spoolman_spool. Only writes when the request
+    # Persist BambuStudio slicer preset AND color_name under spool.extra.
+    # Spoolman has no native fields for these — color_name was confirmed
+    # absent from the FilamentUpdateParameters schema in 0.23.1 (#1357), so
+    # writing `filament.color_name` was a silent no-op that left every
+    # edit looking "not saved". They all round-trip via extra and get
+    # unpacked in _map_spoolman_spool. Only writes when the request
    # explicitly set the field — passing null/omitting leaves the existing
    # extra entry untouched (write empty string to clear).
    sf_set = "slicer_filament" in data.model_fields_set
    sfn_set = "slicer_filament_name" in data.model_fields_set
-    if sf_set or sfn_set:
+    cn_set = "color_name" in data.model_fields_set
+    if sf_set or sfn_set or cn_set:
        # Ensure extra fields are registered (Spoolman rejects PATCHes with
        # unknown keys with HTTP 400). Idempotent if startup already ran this.
        if sf_set:
            await client.ensure_extra_field("bambu_slicer_filament")
        if sfn_set:
            await client.ensure_extra_field("bambu_slicer_filament_name")
+        if cn_set:
+            await client.ensure_extra_field("bambu_color_name")
        new_extra: dict = {}
        if sf_set:
            new_extra["bambu_slicer_filament"] = json.dumps(data.slicer_filament or "")
        if sfn_set:
            new_extra["bambu_slicer_filament_name"] = json.dumps(data.slicer_filament_name or "")
+        if cn_set:
+            new_extra["bambu_color_name"] = json.dumps(data.color_name or "")
        async with _translate_spoolman_errors():
            updated = await client.merge_spool_extra(spool_id, new_extra)

@@ -75,6 +75,25 @@ class SpoolmanClientError(Exception):
        self.response_text = response_text


+def _filament_subtype_part(name: str, material: str) -> str:
+    """Return the subtype portion of a filament name, lowercased.
+
+    Mirrors the read-side derivation in
+    ``backend/app/api/routes/_spoolman_helpers.py::_map_spoolman_spool``:
+    if the filament name starts with the material prefix (e.g. ``"PLA Glow"``
+    when material is ``"PLA"``), strip it; otherwise return the name as-is.
+
+    Used by ``find_or_create_filament`` so that an existing filament saved by
+    the AMS-sync path with name ``"Glow"`` still matches a user-driven edit
+    that composes ``"PLA Glow"`` (#1357).
+    """
+    s = (name or "").strip()
+    m = (material or "").strip()
+    if m and s.upper().startswith(m.upper() + " "):
+        return s[len(m) + 1 :].strip().lower()
+    return s.lower()
+
+
 class SpoolmanClient:
    """Client for interacting with Spoolman API."""

@@ -623,48 +642,58 @@ class SpoolmanClient:
        if brand:
            vendor_id = await self.find_or_create_vendor(brand)

+        # Normalised match keys (case-insensitive). Computed once outside the
+        # loop so the inner comparison stays simple.
+        composed_subtype = _filament_subtype_part(name, material)
+        material_norm = material.upper()
+        brand_norm = (brand or "").strip().lower()
+
        filaments = await self.get_filaments()
        for f in filaments:
            f_material = (f.get("material") or "").upper()
-            f_name = (f.get("name") or "").strip()
            f_color = (f.get("color_hex") or "").upper()[:6]
            f_vendor = f.get("vendor") or {}
            f_vendor_name = (f_vendor.get("name") or "").strip().lower()

-            material_match = f_material == material.upper()
-            name_match = f_name.lower() == name.lower()
+            material_match = f_material == material_norm
+            # Match on the subtype portion of the filament name. AMS-sync
+            # auto-create (the underscore-prefixed `_find_or_create_filament`
+            # used during MQTT tray import) stores the filament as just
+            # ``tray.tray_sub_brands`` — e.g. ``"Glow"`` — while the
+            # user-driven edit path here composes ``"<material> <subtype>"``
+            # — ``"PLA Glow"``. The old literal equality `f_name == name`
+            # failed to bridge the two shapes, so every edit fell through to
+            # `create_filament`, leaving a trail of duplicate filaments AND
+            # leaving the spool either still pointed at the old filament
+            # whose `color_name` never got patched, or pointed at a new
+            # filament with the colour while the inventory list kept
+            # showing the synth fallback from the old one (#1357).
+            f_subtype_part = _filament_subtype_part(f.get("name") or "", material)
+            name_match = f_subtype_part == composed_subtype
            color_match = f_color == color
-            vendor_match = (not brand) or f_vendor_name == (brand or "").strip().lower()
+            vendor_match = (not brand) or f_vendor_name == brand_norm

            if material_match and name_match and color_match and vendor_match:
-                # #1319: color_name is not part of the match key, but if the
-                # caller passed a value that differs from what's stored, update
-                # the filament — otherwise the user's edit is silently dropped
-                # and the inventory read falls back to subtype, making it look
-                # like color_name "reverts" to the subtype on every save.
-                # Convention: None = "don't touch"; "" = explicit clear; any
-                # other string = set/update.
-                if color_name is not None:
-                    existing = (f.get("color_name") or "").strip()
-                    requested = color_name.strip()
-                    if requested != existing:
-                        payload_value: str | None = requested if requested else None
-                        try:
-                            await self.patch_filament(f["id"], {"color_name": payload_value})
-                        except Exception as e:
-                            logger.warning(
-                                "Failed to update color_name on filament %s: %s",
-                                f["id"],
-                                e,
-                            )
+                # color_name is intentionally not part of the match key and
+                # is no longer patched onto the filament here: Spoolman 0.23.1
+                # has no `color_name` field on Filament (#1357 — confirmed
+                # against the FilamentUpdateParameters schema). The earlier
+                # #1319 fix tried to patch it and Spoolman silently dropped
+                # the key, which is exactly why the user's edit looked "not
+                # saved". The route now persists color_name via
+                # spool.extra.bambu_color_name (see _map_spoolman_spool for
+                # the read side); find_or_create_filament's only job is to
+                # resolve the right filament_id for the spool link.
                return f["id"]

+        # color_name omitted: Spoolman has no such field on Filament (#1357);
+        # the user's color_name lands in spool.extra.bambu_color_name via the
+        # route after find_or_create_filament returns the new id.
        filament = await self.create_filament(
            name=name,
            vendor_id=vendor_id,
            material=material,
            color_hex=color,
-            color_name=color_name,
            weight=float(label_weight),
        )
        filament_id = filament.get("id")
@@ -66,6 +66,7 @@ def mock_spoolman_client():
    mock_client.update_spool_full = AsyncMock(return_value=SAMPLE_SPOOLMAN_SPOOL)
    mock_client.merge_spool_extra = AsyncMock(return_value=SAMPLE_SPOOLMAN_SPOOL)
    mock_client.find_or_create_filament = AsyncMock(return_value=7)
+    mock_client.ensure_extra_field = AsyncMock(return_value=True)

    with (
        patch(
@@ -323,39 +324,50 @@ class TestSpoolmanInventoryCRUD:

    @pytest.mark.asyncio
    @pytest.mark.integration
-    async def test_update_with_explicit_null_color_name_clears(
+    async def test_update_with_explicit_null_color_name_clears_extra(
        self,
        async_client: AsyncClient,
        spoolman_settings,
        mock_spoolman_client,
    ):
-        """#1319 follow-up: explicit color_name=null in the PATCH body means
-        "clear" — route translates it to "" so find_or_create_filament patches
-        the matched filament with color_name=None."""
+        """#1357: explicit color_name=null means "clear". The route writes a
+        JSON-encoded empty string to spool.extra.bambu_color_name so the read
+        path falls back to the synth value next time."""
        payload = {"color_name": None}
        response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload)
        assert response.status_code == 200
-        mock_spoolman_client.find_or_create_filament.assert_called_once()
-        kwargs = mock_spoolman_client.find_or_create_filament.call_args.kwargs
-        assert kwargs["color_name"] == ""
+        mock_spoolman_client.ensure_extra_field.assert_any_call("bambu_color_name")
+        mock_spoolman_client.merge_spool_extra.assert_called_once()
+        _, kwargs = mock_spoolman_client.merge_spool_extra.call_args
+        # First positional arg is spool_id; second is the extra-dict patch.
+        args = mock_spoolman_client.merge_spool_extra.call_args.args
+        extra_patch = args[1] if len(args) > 1 else kwargs.get("new_fields", {})
+        import json as _json
+
+        assert _json.loads(extra_patch["bambu_color_name"]) == ""

    @pytest.mark.asyncio
    @pytest.mark.integration
-    async def test_update_without_color_name_keeps_current(
+    async def test_update_without_color_name_skips_extra_write(
        self,
        async_client: AsyncClient,
        spoolman_settings,
        mock_spoolman_client,
    ):
-        """#1319 follow-up: when color_name is omitted from the PATCH body the
-        current value is kept — None passed to find_or_create_filament means
-        "don't touch"."""
+        """#1357: when color_name is omitted from the PATCH body the extra
+        write is skipped entirely — no merge_spool_extra call, no ensure_extra
+        call for bambu_color_name. Only fields the request explicitly set go
+        through the extra round-trip."""
        payload = {"note": "only updating note"}
        response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload)
        assert response.status_code == 200
-        kwargs = mock_spoolman_client.find_or_create_filament.call_args.kwargs
-        # SAMPLE_SPOOLMAN_SPOOL has no color_name field, so cur fallback is None.
-        assert kwargs["color_name"] is None
+        # No call should target bambu_color_name when color_name wasn't in the body.
+        color_name_calls = [
+            c
+            for c in mock_spoolman_client.ensure_extra_field.call_args_list
+            if c.args and c.args[0] == "bambu_color_name"
+        ]
+        assert color_name_calls == []

    @pytest.mark.asyncio
    @pytest.mark.integration
@@ -1117,17 +1129,26 @@ class TestStorageLocationPassthrough:


 class TestColorNamePassthrough:
-    """color_name is forwarded to find_or_create_filament on create and update (B6 / T5)."""
+    """color_name persistence via spool.extra.bambu_color_name (#1357).
+
+    Spoolman 0.23.1 has no `color_name` field on Filament, so Bambuddy owns
+    the round-trip via the spool's extra dict — same shape as the existing
+    bambu_slicer_filament storage. These tests pin that the create/update
+    routes register the extra field and write to merge_spool_extra, NOT to
+    find_or_create_filament's color_name parameter.
+    """

    @pytest.mark.asyncio
    @pytest.mark.integration
-    async def test_create_passes_color_name_to_filament(
+    async def test_create_writes_color_name_to_spool_extra(
        self,
        async_client: AsyncClient,
        spoolman_settings,
        mock_spoolman_client,
    ):
-        """color_name from the create payload is forwarded to find_or_create_filament."""
+        """color_name from create payload lands in spool.extra.bambu_color_name."""
+        import json as _json
+
        payload = {
            "material": "PLA",
            "label_weight": 1000,
@@ -1136,41 +1157,53 @@ class TestColorNamePassthrough:
        }
        response = await async_client.post("/api/v1/spoolman/inventory/spools", json=payload)
        assert response.status_code == 200
-        mock_spoolman_client.find_or_create_filament.assert_called_once()
-        _, kwargs = mock_spoolman_client.find_or_create_filament.call_args
-        assert kwargs.get("color_name") == "Bambu Green"
+        mock_spoolman_client.ensure_extra_field.assert_any_call("bambu_color_name")
+        mock_spoolman_client.merge_spool_extra.assert_called_once()
+        args = mock_spoolman_client.merge_spool_extra.call_args.args
+        extra_patch = args[1]
+        assert _json.loads(extra_patch["bambu_color_name"]) == "Bambu Green"

    @pytest.mark.asyncio
    @pytest.mark.integration
-    async def test_update_passes_color_name_to_filament(
+    async def test_update_writes_color_name_to_spool_extra(
        self,
        async_client: AsyncClient,
        spoolman_settings,
        mock_spoolman_client,
    ):
-        """color_name from the update payload is forwarded to find_or_create_filament."""
+        """color_name from update payload lands in spool.extra.bambu_color_name —
+        this is the #1357 reproduction: previously the value went to
+        filament.color_name which Spoolman silently dropped."""
+        import json as _json
+
        payload = {"color_name": "Jade White"}
        response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload)
        assert response.status_code == 200
-        mock_spoolman_client.find_or_create_filament.assert_called_once()
-        _, kwargs = mock_spoolman_client.find_or_create_filament.call_args
-        assert kwargs.get("color_name") == "Jade White"
+        mock_spoolman_client.ensure_extra_field.assert_any_call("bambu_color_name")
+        mock_spoolman_client.merge_spool_extra.assert_called_once()
+        args = mock_spoolman_client.merge_spool_extra.call_args.args
+        extra_patch = args[1]
+        assert _json.loads(extra_patch["bambu_color_name"]) == "Jade White"

    @pytest.mark.asyncio
    @pytest.mark.integration
-    async def test_update_omits_color_name_when_not_provided(
+    async def test_update_omits_color_name_skips_extra_write(
        self,
        async_client: AsyncClient,
        spoolman_settings,
        mock_spoolman_client,
    ):
-        """When color_name is not in the PATCH payload, the existing filament color_name is used."""
+        """When color_name is absent from the PATCH body, the route must not
+        write to spool.extra at all (preserves any existing value)."""
        payload = {"note": "no color_name here"}
        response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload)
        assert response.status_code == 200
-        _, kwargs = mock_spoolman_client.find_or_create_filament.call_args
-        # color_name falls back to current filament's color_name (which is None in test fixture)
-        assert kwargs.get("color_name") is None
+        color_name_calls = [
+            c
+            for c in mock_spoolman_client.ensure_extra_field.call_args_list
+            if c.args and c.args[0] == "bambu_color_name"
+        ]
+        assert color_name_calls == []


 class TestSpoolmanInventoryAuth:
@@ -218,6 +218,59 @@ class TestMapSpoolmanSpool:
        # color_name falls back to subtype.
        assert result["color_name"] == "Basic Red"

+    def test_color_name_read_from_spool_extra_first(self):
+        """#1357: the canonical store for color_name is
+        spool.extra.bambu_color_name (JSON-encoded). Read priority is
+        extra > filament.color_name > subtype-synth. The user's
+        Bambuddy-saved value MUST win even when Spoolman's own
+        filament.color_name happens to be populated from some other source.
+        """
+        spool = {
+            **MINIMAL_SPOOL,
+            "extra": {"bambu_color_name": '"Galaxy Black"'},
+            "filament": {
+                **MINIMAL_SPOOL["filament"],
+                "name": "PLA Glow",
+                "color_name": "Glow",  # would be picked up if extra weren't preferred
+            },
+        }
+        result = _map_spoolman_spool(spool)
+        assert result["color_name"] == "Galaxy Black"
+        assert result["color_name_is_synthesized"] is False
+
+    def test_color_name_empty_extra_falls_through_to_filament(self):
+        """An explicit empty string in spool.extra.bambu_color_name (the
+        "user cleared the field" shape) must NOT mask Spoolman's own
+        filament.color_name if one exists — it falls through to the next
+        layer instead of suppressing it."""
+        spool = {
+            **MINIMAL_SPOOL,
+            "extra": {"bambu_color_name": '""'},
+            "filament": {
+                **MINIMAL_SPOOL["filament"],
+                "color_name": "Sunset",
+            },
+        }
+        result = _map_spoolman_spool(spool)
+        assert result["color_name"] == "Sunset"
+        assert result["color_name_is_synthesized"] is False
+
+    def test_color_name_empty_extra_falls_through_to_synth(self):
+        """When extra is cleared and filament has no color_name either,
+        fall all the way through to the subtype synth — same UX as a fresh
+        Spoolman install."""
+        spool = {
+            **MINIMAL_SPOOL,
+            "extra": {"bambu_color_name": '""'},
+            "filament": {
+                **MINIMAL_SPOOL["filament"],
+                "name": "PLA Basic Red",
+            },
+        }
+        result = _map_spoolman_spool(spool)
+        assert result["color_name"] == "Basic Red"
+        assert result["color_name_is_synthesized"] is True
+
    def test_color_name_none_when_both_fields_empty(self):
        """If neither color_name nor a usable subtype exists, return None — UI
        falls back to its own 'Unknown color' string rather than showing a
@@ -323,26 +323,15 @@ class TestFindOrCreateFilament:
        assert result == 7

    @pytest.mark.asyncio
-    async def test_patches_color_name_on_existing_filament_when_changed(self, client):
-        """#1319: color_name is not part of the match key, so when a caller
-        updates a spool with a new color_name and the material/name/color/vendor
-        still match an existing filament, the existing filament's color_name
-        must be patched — otherwise the user's edit is silently dropped."""
-        existing = {**SAMPLE_FILAMENT, "color_name": None}
-        with (
-            patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)),
-            patch.object(client, "get_filaments", AsyncMock(return_value=[existing])),
-            patch.object(client, "patch_filament", AsyncMock(return_value={"id": 7})) as mock_patch,
-        ):
-            result = await client.find_or_create_filament(
-                "PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="Sunny Yellow"
-            )
-        assert result == 7
-        mock_patch.assert_called_once_with(7, {"color_name": "Sunny Yellow"})
-
-    @pytest.mark.asyncio
-    async def test_does_not_patch_when_color_name_unchanged(self, client):
-        existing = {**SAMPLE_FILAMENT, "color_name": "Sunny Yellow"}
+    async def test_color_name_does_not_trigger_filament_patch(self, client):
+        """#1357: Spoolman 0.23.1 has no `color_name` field on Filament
+        (verified against FilamentUpdateParameters schema). find_or_create_filament
+        must NOT attempt to PATCH it — the route now persists the user's
+        color_name to spool.extra.bambu_color_name instead. Any patch call
+        from this layer would be a silent no-op (Spoolman ignores unknown
+        keys) and was the original symptom of "edits never save".
+        """
+        existing = {**SAMPLE_FILAMENT}
        with (
            patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)),
            patch.object(client, "get_filaments", AsyncMock(return_value=[existing])),
@@ -355,47 +344,88 @@ class TestFindOrCreateFilament:
        mock_patch.assert_not_called()

    @pytest.mark.asyncio
-    async def test_does_not_patch_when_color_name_empty(self, client):
-        """An empty/None color_name should not clobber an existing value."""
-        existing = {**SAMPLE_FILAMENT, "color_name": "Sunny Yellow"}
+    async def test_matches_filament_named_with_just_subtype(self, client):
+        """#1357: AMS-sync auto-create saves the filament with name set to just
+        ``tray.tray_sub_brands`` (e.g. ``"Glow"`` without the material prefix),
+        but the user-driven edit path composes ``"<material> <subtype>"``
+        (``"PLA Glow"``). Before this fix the literal `f_name == name` check
+        failed to bridge the two shapes, so every edit fell through to
+        ``create_filament`` and left a trail of duplicate filaments. Now the
+        name match strips the material prefix on both sides, so the two
+        shapes resolve to the same subtype key."""
+        existing = {
+            **SAMPLE_FILAMENT,
+            "id": 11,
+            "name": "Glow",  # AMS-sync shape: just subtype
+            "material": "PLA",
+            "color_hex": "AAF3C6",
+            "color_name": None,
+            "vendor": {"id": 3, "name": "Amazon Basics"},
+        }
        with (
            patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)),
            patch.object(client, "get_filaments", AsyncMock(return_value=[existing])),
            patch.object(client, "patch_filament", AsyncMock()) as mock_patch,
-        ):
-            result = await client.find_or_create_filament("PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name=None)
-        assert result == 7
-        mock_patch.assert_not_called()
-
-    @pytest.mark.asyncio
-    async def test_clears_color_name_when_empty_string_passed(self, client):
-        """#1319 follow-up: empty string means "explicit clear" — the route
-        layer translates a wire-level null into "" so the user can blank the
-        field on a previously-set spool."""
-        existing = {**SAMPLE_FILAMENT, "color_name": "Sunny Yellow"}
-        with (
-            patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)),
-            patch.object(client, "get_filaments", AsyncMock(return_value=[existing])),
-            patch.object(client, "patch_filament", AsyncMock(return_value={"id": 7})) as mock_patch,
-        ):
-            result = await client.find_or_create_filament("PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="")
-        assert result == 7
-        mock_patch.assert_called_once_with(7, {"color_name": None})
-
-    @pytest.mark.asyncio
-    async def test_patch_failure_does_not_block_match(self, client):
-        """A patch_filament failure must not prevent returning the matched id —
-        save should still link the spool to the correct filament."""
-        existing = {**SAMPLE_FILAMENT, "color_name": None}
-        with (
-            patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)),
-            patch.object(client, "get_filaments", AsyncMock(return_value=[existing])),
-            patch.object(client, "patch_filament", AsyncMock(side_effect=SpoolmanUnavailableError("boom"))),
+            patch.object(client, "create_filament", AsyncMock()) as mock_create,
        ):
            result = await client.find_or_create_filament(
-                "PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="Sunny Yellow"
+                "PLA", "Glow", "Amazon Basics", "AAF3C6", 1000, color_name="Bright Glow"
+            )
+        assert result == 11
+        # color_name is no longer written via the filament — see #1357 — and
+        # the function must not create a duplicate filament.
+        mock_patch.assert_not_called()
+        mock_create.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_still_matches_filament_named_material_plus_subtype(self, client):
+        """The composed-name shape (``"PLA Basic"`` matching a Spoolman filament
+        also named ``"PLA Basic"``) must keep working — the normalisation strips
+        the prefix on both sides, so the comparison is on the subtype part."""
+        existing = {
+            **SAMPLE_FILAMENT,
+            "id": 7,
+            "name": "PLA Basic",
+            "material": "PLA",
+            "color_hex": "FF0000",
+            "color_name": "Sunset",
+        }
+        with (
+            patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)),
+            patch.object(client, "get_filaments", AsyncMock(return_value=[existing])),
+            patch.object(client, "patch_filament", AsyncMock(return_value={"id": 7})),
+            patch.object(client, "create_filament", AsyncMock()) as mock_create,
+        ):
+            result = await client.find_or_create_filament(
+                "PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="Sunset"
            )
        assert result == 7
+        mock_create.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_name_match_does_not_cross_materials(self, client):
+        """Sanity check: a filament with name=subtype must NOT match a request
+        with a different material that happens to share the subtype string.
+        material_match runs first and fails, so the iteration moves on and
+        ``create_filament`` is called."""
+        existing = {
+            **SAMPLE_FILAMENT,
+            "id": 7,
+            "name": "Basic",
+            "material": "PETG",  # different material
+            "color_hex": "FF0000",
+        }
+        new_filament = {"id": 99, "name": "PLA Basic"}
+        with (
+            patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)),
+            patch.object(client, "get_filaments", AsyncMock(return_value=[existing])),
+            patch.object(client, "create_filament", AsyncMock(return_value=new_filament)) as mock_create,
+        ):
+            result = await client.find_or_create_filament(
+                "PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="Sunset"
+            )
+        assert result == 99
+        mock_create.assert_called_once()

    @pytest.mark.asyncio
    async def test_creates_filament_when_no_match(self, client):
@@ -407,12 +437,15 @@ class TestFindOrCreateFilament:
        ):
            result = await client.find_or_create_filament("PETG", "Pro", "Bambu Lab", "00FF00", 1000)
        assert result == 99
+        # color_name is intentionally not forwarded to create_filament (#1357):
+        # Spoolman has no such field on Filament, so passing it would be a
+        # no-op. The route persists color_name to spool.extra.bambu_color_name
+        # after this returns.
        mock_create.assert_called_once_with(
            name="PETG Pro",
            vendor_id=3,
            material="PETG",
            color_hex="00FF00",
-            color_name=None,
            weight=1000.0,
        )

@@ -443,7 +476,6 @@ class TestFindOrCreateFilament:
            vendor_id=None,
            material="ABS",
            color_hex="FF0000",
-            color_name=None,
            weight=750.0,
        )