Merge pull request #46 from mbillow/claude/issue-27-investigation-ff60pw

fix(fridge,observe): merge partial cache updates, name ice makers dynamically

custom_components/localthings/entity.py

@@ -8,7 +8,7 @@ from homeassistant.helpers.device_registry import DeviceInfo
 from homeassistant.const import EntityCategory
 
 from .registry.adapter import _key
-from .registry.discovery import BoundEntity
+from .registry.discovery import BoundEntity, _snake_to_title
 
 from .const import DOMAIN
 from .coordinator import LocalThingsCoordinator
@@ -44,7 +44,7 @@ def _derive_name(state_key: str) -> str:
     instance number with a space: "door_cooler_open1" → "Door Cooler Open 1".
     """
     name = re.sub(r'(\d+)$', lambda m: f' {m.group()}' if int(m.group()) > 0 else '', state_key)
-    return name.replace('_', ' ').title().strip()
+    return _snake_to_title(name).strip()
 
 
 class LocalThingsEntity(CoordinatorEntity[LocalThingsCoordinator]):
@@ -57,7 +57,15 @@ class LocalThingsEntity(CoordinatorEntity[LocalThingsCoordinator]):
         self._bound = bound
         self._state_key = _key(bound)
         self._attr_unique_id = f"{DOMAIN}_{coordinator.device_serial}_{self._state_key}"
-        self._attr_name = bound.desc.name if bound.desc.name is not None else _derive_name(self._state_key)
+        if bound.desc.name is not None:
+            self._attr_name = bound.desc.name
+        elif bound.instance_name:
+            # A device-given instance name (e.g. an ice maker's "Cubed
+            # Ice") takes the place of the href-derived instance label,
+            # keeping the same entity-specific suffix (issue #27).
+            self._attr_name = f"{bound.instance_name} {_derive_name(bound.desc.key)}".strip()
+        else:
+            self._attr_name = _derive_name(self._state_key)
         self._attr_translation_key = bound.desc.translation_key
         self._attr_icon = bound.desc.icon
         raw_cat = bound.desc.entity_category

custom_components/localthings/observe.py

@@ -69,6 +69,7 @@ class ObserveManager:
         self.last_mode_change_wall = time.time()
         self._settle_until: dict[str, float] = {}
         self._settle_lock = threading.Lock()
+        self._cache_lock = threading.Lock()
         self.subscribed_hrefs: set[str] = set()
         self._notified: set[str] = set()
         self._last_notify_ts: float | None = None
@@ -92,11 +93,35 @@ class ObserveManager:
             return True
 
     def apply(self, href: str, rep: dict, source: str) -> bool:
-        """Gate a StateCache.apply_rep call through the write-settle guard."""
+        """Gate a StateCache.apply_rep call through the write-settle guard.
+
+        Merges the incoming rep onto whatever's already cached for this
+        href rather than handing it to StateCache.apply_rep verbatim --
+        apply_rep does a full replace, and Samsung devices don't always
+        repeat every field on every update (issue #27: a /mode/vs/0
+        OBSERVE notify -- and, on at least one Bespoke fridge, even the
+        /device/0 sweep entry for that href -- can carry just `modes`,
+        omitting `supportedOptions`/`supportedModes` entirely). A full
+        replace would silently wipe fields a select entity's
+        exists_fn/options_field gates on the moment one partial update
+        comes through, even though nothing about the device's actual
+        supported options changed.
+
+        `apply()` is the sole path StateCache mutations flow through in
+        this component (poll, sweep, and OBSERVE notify all funnel here),
+        so `_cache_lock` serializes the read-then-write across those
+        threads (DTLS reader for notifies, executor threads for poll/
+        sweep) -- without it, two concurrent updates for the same href
+        could each read the same prior rep and the second writer would
+        silently lose the first's fields, reintroducing the exact bug
+        this merge fixes.
+        """
         if self._is_settling(href):
             self.log.debug("dropping %s update for %s (settling)", source, href)
             return False
-        return self.cache.apply_rep(href, rep, source=source)
+        with self._cache_lock:
+            merged = {**(self.cache.get(href) or {}), **rep}
+            return self.cache.apply_rep(href, merged, source=source)
 
     def on_notification(self, href: str, payload: bytes) -> None:
         """Wired as DtlsCoapSession.on_notification. Runs on the DTLS

custom_components/localthings/registry/capabilities/fridge.py

@@ -100,6 +100,12 @@ ICEMAKER_NIGHTTIME = Capability(
 # Icemaker (generic — covers /icemaker/one/vs/0, /icemaker/two/vs/0)
 # /icemaker/status/vs/0 is kept as exact-href cap and binds first.
 # /icemaker/nighttime/vs/0 is excluded by match_fn (lacks iceMaker.state).
+#
+# Entity names are derived from x.com.samsung.da.iceMaker.name ("CUBED_ICE",
+# "ICE_BITES") via name_field, not the href's "one"/"two" segment -- these two
+# ice makers are independent on/off toggles that can both be enabled at once
+# (issue #27), so they stay separate entities rather than a single ice-type
+# select, but users still want them labeled with the device's own names.
 # ---------------------------------------------------------------------------
 
 def _icemaker_write(field):
@@ -112,6 +118,7 @@ ICEMAKER_GENERIC = Capability(
     href=None,
     href_prefix='/icemaker/',
     match_fn=lambda rep, resources: 'x.com.samsung.da.iceMaker.state' in rep,
+    name_field='x.com.samsung.da.iceMaker.name',
     poll_tier='warm',
     entities=(
         SensorDesc(key='making_status',

custom_components/localthings/registry/capability.py

@@ -15,6 +15,11 @@ class Capability:
     rt_filter: Optional[str] = None          # bind only if rt_filter in rep.get('rt', ())
     href_prefix: Optional[str] = None        # pattern caps only: bind only if href starts with this
     strip_prefix_in_key: bool = False         # strip href_prefix segs before building key_override
+    # Rep field holding this instance's device-given name (e.g. an ice
+    # maker's "CUBED_ICE"/"ICE_BITES"), normalized and used as the display
+    # name prefix in place of the href-derived instance label. Does not
+    # affect key_override/unique_id -- only what's shown in the UI.
+    name_field: Optional[str] = None
     match_fn: Optional[Callable[[dict, dict], bool]] = None  # match_fn(rep, resources) -> bool
     # Rare optional hook — only operational-state-style resources use this.
     on_observation: Optional[Callable[[dict, dict], None]] = None

custom_components/localthings/registry/discovery.py

@@ -27,6 +27,25 @@ class BoundEntity:
     desc: SamsungEntityDescription
     instance: str = ''
     key_override: Optional[str] = None
+    instance_name: Optional[str] = None
+
+
+def _snake_to_title(s: str) -> str:
+    """'CUBED_ICE'/'cubed_ice' -> 'Cubed Ice'. Shared with entity.py's
+    _derive_name, which applies the same transform to a state key."""
+    return s.replace('_', ' ').title()
+
+
+def _instance_name(cap: Capability, rep: dict) -> Optional[str]:
+    """Normalize `cap.name_field`'s raw value ("CUBED_ICE" -> "Cubed Ice")
+    for use as a display-name prefix, or None if the cap doesn't declare
+    one or the device didn't report it."""
+    if not cap.name_field:
+        return None
+    raw = rep.get(cap.name_field)
+    if not isinstance(raw, str) or not raw:
+        return None
+    return _snake_to_title(raw)
 
 
 def instance_suffix(href: str) -> str:
@@ -37,6 +56,18 @@ def instance_suffix(href: str) -> str:
     return ''
 
 
+def _bind(cap: Capability, href: str, inst: str, inst_name: Optional[str],
+          key_prefix: Optional[str] = None) -> list[BoundEntity]:
+    """Build one BoundEntity per entity on `cap`, sharing the instance/
+    key-prefix/instance-name computed once by the caller."""
+    return [
+        BoundEntity(href=href, capability=cap, desc=desc, instance=inst,
+                    key_override=f'{key_prefix}_{desc.key}' if key_prefix else None,
+                    instance_name=inst_name)
+        for desc in cap.entities
+    ]
+
+
 def discover(
     resources: dict[str, dict],
     registry: dict[str, list[Capability]],
@@ -58,9 +89,7 @@ def discover(
             if cap.match_fn is not None and not cap.match_fn(rep, resources):
                 continue
             inst = instance_suffix(href)
-            for desc in cap.entities:
-                out.append(BoundEntity(href=href, capability=cap,
-                                       desc=desc, instance=inst))
+            out.extend(_bind(cap, href, inst, _instance_name(cap, rep)))
             matched = True
 
         if matched:
@@ -78,10 +107,7 @@ def discover(
             # Auto-derive key prefix from href segments (skip digits and 'vs')
             src = href[len(cap.href_prefix):] if (cap.strip_prefix_in_key and cap.href_prefix) else href
             segs = [s for s in src.strip('/').split('/') if s and not s.isdigit() and s != 'vs']
-            for desc in cap.entities:
-                key_override = '_'.join(segs) + '_' + desc.key
-                out.append(BoundEntity(href=href, capability=cap, desc=desc,
-                                       instance=inst, key_override=key_override))
+            out.extend(_bind(cap, href, inst, _instance_name(cap, rep), '_'.join(segs)))
             matched = True
             break
 

tests/localthings/test_observe.py

@@ -34,6 +34,28 @@ def test_apply_writes_through_when_not_settling():
     assert mgr.cache.get('/oven/vs/0') == {'a': 1}
 
 
+def test_apply_merges_partial_update_onto_prior_rep():
+    """Regression test for issue #27: a Bespoke fridge's /mode/vs/0 notify
+    (and even a later sweep entry for that href) can carry only `modes`,
+    omitting `supportedOptions` entirely. A full replace would wipe
+    `supportedOptions` from the cache the moment that partial update
+    arrives, even though the device's supported options didn't change --
+    which is exactly what made the flex-zone select disappear."""
+    mgr = _manager()
+    full = {
+        'x.com.samsung.da.modes': ['CVN_CONVERTIBLE_ZONE', 'CV_FDR_MEAT'],
+        'x.com.samsung.da.supportedOptions': ['CV_FDR_WINE', 'CV_FDR_MEAT'],
+    }
+    mgr.apply('/mode/vs/0', full, source='poll')
+
+    partial = {'x.com.samsung.da.modes': ['CVN_CONVERTIBLE_ZONE', 'WATERFILTER_ENABLE']}
+    mgr.apply('/mode/vs/0', partial, source='observe')
+
+    cached = mgr.cache.get('/mode/vs/0')
+    assert cached['x.com.samsung.da.modes'] == ['CVN_CONVERTIBLE_ZONE', 'WATERFILTER_ENABLE']
+    assert cached['x.com.samsung.da.supportedOptions'] == ['CV_FDR_WINE', 'CV_FDR_MEAT']
+
+
 def test_apply_drops_update_during_settle_window():
     mgr = _manager()
     mgr.cache.apply_rep('/oven/vs/0', {'a': 1}, source='seed')

tests/test_discovery.py

@@ -158,6 +158,46 @@ def test_discover_match_fn_decline_is_not_logged_as_gap():
     assert seen == []
 
 
+def test_discover_pattern_cap_reads_name_field():
+    """A pattern cap's name_field normalizes a device-given instance name
+    (e.g. an ice maker's "CUBED_ICE") for use in the entity's display name,
+    independent of the href-derived key (issue #27)."""
+    cap = Capability(
+        href=None,
+        href_prefix='/icemaker/',
+        name_field='x.com.samsung.da.iceMaker.name',
+        entities=(BinarySensorDesc(key='enabled', field='x.com.samsung.da.iceMaker.state'),),
+    )
+    resources = {
+        '/icemaker/one/vs/0': {
+            'x.com.samsung.da.iceMaker.state': 'On',
+            'x.com.samsung.da.iceMaker.name': 'CUBED_ICE',
+        },
+        '/icemaker/two/vs/0': {
+            'x.com.samsung.da.iceMaker.state': 'On',
+            'x.com.samsung.da.iceMaker.name': 'ICE_BITES',
+        },
+    }
+    bound = discover(resources, {}, pattern_caps=[cap])
+    names = {b.href: b.instance_name for b in bound}
+    assert names == {
+        '/icemaker/one/vs/0': 'Cubed Ice',
+        '/icemaker/two/vs/0': 'Ice Bites',
+    }
+
+
+def test_discover_pattern_cap_name_field_absent_leaves_instance_name_none():
+    cap = Capability(
+        href=None,
+        href_prefix='/icemaker/',
+        name_field='x.com.samsung.da.iceMaker.name',
+        entities=(BinarySensorDesc(key='enabled', field='x.com.samsung.da.iceMaker.state'),),
+    )
+    resources = {'/icemaker/one/vs/0': {'x.com.samsung.da.iceMaker.state': 'On'}}
+    bound = discover(resources, {}, pattern_caps=[cap])
+    assert bound[0].instance_name is None
+
+
 def test_discover_rt_filter_gates_binding():
     """Cap with rt_filter must not bind a rep whose rt list does not match."""
     oven_mode_cap = Capability(

tests/test_entity_naming.py

@@ -0,0 +1,45 @@
+"""Tests for LocalThingsEntity's display-name derivation
+(custom_components/localthings/entity.py) -- the explicit-name,
+device-given-instance-name, and href-derived fallbacks.
+"""
+from custom_components.localthings.entity import LocalThingsEntity
+from custom_components.localthings.registry.capability import Capability
+from custom_components.localthings.registry.discovery import BoundEntity
+from custom_components.localthings.registry.entities import BinarySensorDesc
+
+
+class _FakeCoordinator:
+    device_serial = 'TEST-SERIAL'
+
+    def __init__(self, last_resources=None):
+        self.last_resources = last_resources or {}
+
+
+def _make_entity(desc, href='/x/vs/0', key_override=None, instance='', instance_name=None):
+    capability = Capability(href=href, entities=(desc,))
+    bound = BoundEntity(href=href, capability=capability, desc=desc,
+                         instance=instance, key_override=key_override,
+                         instance_name=instance_name)
+    return LocalThingsEntity(_FakeCoordinator(), bound)
+
+
+def test_explicit_name_wins_over_everything():
+    desc = BinarySensorDesc(key='enabled', name='Explicit Name')
+    entity = _make_entity(desc, instance_name='Cubed Ice')
+    assert entity._attr_name == 'Explicit Name'
+
+
+def test_instance_name_prefixes_the_derived_suffix():
+    """Issue #27: an ice maker's device-given name ("Cubed Ice") replaces
+    the href-derived instance label ("Icemaker One") as the name prefix,
+    keeping the same entity-specific suffix."""
+    desc = BinarySensorDesc(key='enabled')
+    entity = _make_entity(desc, key_override='icemaker_one_enabled',
+                           instance_name='Cubed Ice')
+    assert entity._attr_name == 'Cubed Ice Enabled'
+
+
+def test_no_instance_name_falls_back_to_derived_state_key():
+    desc = BinarySensorDesc(key='enabled')
+    entity = _make_entity(desc, key_override='icemaker_one_enabled')
+    assert entity._attr_name == 'Icemaker One Enabled'