Address Claude Code review: bounded caches, MP4 cleanup, avoid double-read

#2 (weak sentinel): _ensure_viz_deps() now checks all five key packages
(matplotlib, imageio, h5py, textual, dash) so a user with matplotlib
pre-installed (Anaconda, etc.) but missing the other viz deps still
triggers the install rather than getting raw ImportErrors later.

#5 (duplicate cache): Extract bounded FIFO cache to _step_cache.py with
load(), seed(), and CACHE_MAX. tui.py and interactive.py now import from
the shared module. test_viz.py TestTuiCache tests _step_cache directly
and gains a test_seed_clears_and_populates case.

Also removes unused Dict import from tui.py (no longer needed after
the _cache type annotation moved to _step_cache.py).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: sbryngelson

toolchain/mfc/viz/_step_cache.py

@@ -0,0 +1,32 @@
+"""Bounded FIFO step cache shared by tui.py and interactive.py.
+
+Keeps up to CACHE_MAX assembled timesteps in memory, evicting the oldest
+entry when the cap is reached.  The module-level state is intentional:
+both the TUI and the interactive server are single-instance; a per-session
+cache avoids redundant disk reads while bounding peak memory usage.
+"""
+
+from typing import Callable
+
+CACHE_MAX: int = 50
+_cache: dict = {}
+_cache_order: list = []
+
+
+def load(step: int, read_func: Callable) -> object:
+    """Return cached data for *step*, calling *read_func* on a miss."""
+    if step not in _cache:
+        if len(_cache) >= CACHE_MAX:
+            evict = _cache_order.pop(0)
+            _cache.pop(evict, None)
+        _cache[step] = read_func(step)
+        _cache_order.append(step)
+    return _cache[step]
+
+
+def seed(step: int, data: object) -> None:
+    """Clear the cache and pre-populate it with already-loaded data."""
+    _cache.clear()
+    _cache_order.clear()
+    _cache[step] = data
+    _cache_order.append(step)

toolchain/mfc/viz/interactive.py

@@ -15,6 +15,7 @@
 from dash import Dash, dcc, html, Input, Output, State, callback_context, no_update
 
 from mfc.printer import cons
+from . import _step_cache
 
 # ---------------------------------------------------------------------------
 # Colormaps available in the picker
@@ -51,19 +52,8 @@
 # ---------------------------------------------------------------------------
 # Server-side data cache  {step -> AssembledData}  (bounded to avoid OOM)
 # ---------------------------------------------------------------------------
-_CACHE_MAX = 50
-_cache: dict = {}
-_cache_order: list = []
-
-
-def _load(step: int, read_func: Callable):
-    if step not in _cache:
-        if len(_cache) >= _CACHE_MAX:
-            evict = _cache_order.pop(0)
-            _cache.pop(evict, None)
-        _cache[step] = read_func(step)
-        _cache_order.append(step)
-    return _cache[step]
+_load = _step_cache.load
+_CACHE_MAX = _step_cache.CACHE_MAX
 
 
 # ---------------------------------------------------------------------------

toolchain/mfc/viz/test_viz.py

@@ -453,13 +453,13 @@ def test_typo_suggests_correct(self):
 # ---------------------------------------------------------------------------
 
 class TestTuiCache(unittest.TestCase):
-    """Test that the TUI step cache respects CACHE_MAX."""
+    """Test that the shared step cache respects CACHE_MAX."""
 
     def setUp(self):
-        import mfc.viz.tui as tui_mod
-        self._mod = tui_mod
-        tui_mod._cache.clear()
-        tui_mod._cache_order.clear()
+        import mfc.viz._step_cache as cache_mod
+        self._mod = cache_mod
+        cache_mod._cache.clear()
+        cache_mod._cache_order.clear()
 
     def tearDown(self):
         self._mod._cache.clear()
@@ -470,7 +470,7 @@ def _read(self, step):
 
     def test_cache_stores_entry(self):
         """Loaded step is stored in cache."""
-        self._mod._load(0, self._read)
+        self._mod.load(0, self._read)
         self.assertIn(0, self._mod._cache)
 
     def test_cache_hit_avoids_reload(self):
@@ -479,19 +479,26 @@ def test_cache_hit_avoids_reload(self):
         def counting(step):
             calls[0] += 1
             return step
-        self._mod._load(5, counting)
-        self._mod._load(5, counting)
+        self._mod.load(5, counting)
+        self._mod.load(5, counting)
         self.assertEqual(calls[0], 1)
 
     def test_cache_evicts_oldest_at_cap(self):
         """Oldest entry is evicted when CACHE_MAX is exceeded."""
-        cap = self._mod._CACHE_MAX
+        cap = self._mod.CACHE_MAX
         for i in range(cap + 3):
-            self._mod._load(i, self._read)
+            self._mod.load(i, self._read)
         self.assertLessEqual(len(self._mod._cache), cap)
         self.assertNotIn(0, self._mod._cache)         # first evicted
         self.assertIn(cap + 2, self._mod._cache)      # most recent kept
 
+    def test_seed_clears_and_populates(self):
+        """seed() clears existing cache and pre-loads one entry."""
+        self._mod.load(99, self._read)   # put something in first
+        self._mod.seed(0, "preloaded")
+        self.assertEqual(len(self._mod._cache), 1)
+        self.assertEqual(self._mod._cache[0], "preloaded")
+
 
 # ---------------------------------------------------------------------------
 # Tests: log scale rendering (new feature smoke tests)

toolchain/mfc/viz/tui.py

@@ -11,7 +11,7 @@
 """
 from __future__ import annotations
 
-from typing import Callable, Dict, List, Optional
+from typing import Callable, List, Optional
 
 import numpy as np
 
@@ -30,29 +30,16 @@
 from textual_plotext import PlotextPlot
 
 from mfc.printer import cons
+from . import _step_cache
 
 # Colormaps available via [c] cycling
 _CMAPS: List[str] = [
     'viridis', 'plasma', 'inferno', 'magma', 'cividis',
     'coolwarm', 'RdBu_r', 'seismic', 'gray',
 ]
 
-# ---------------------------------------------------------------------------
-# Step cache  {step -> AssembledData}  (bounded to avoid OOM)
-# ---------------------------------------------------------------------------
-_CACHE_MAX = 50
-_cache: Dict[int, object] = {}
-_cache_order: List[int] = []
-
-
-def _load(step: int, read_func: Callable) -> object:
-    if step not in _cache:
-        if len(_cache) >= _CACHE_MAX:
-            evict = _cache_order.pop(0)
-            _cache.pop(evict, None)
-        _cache[step] = read_func(step)
-        _cache_order.append(step)
-    return _cache[step]
+_load = _step_cache.load
+_CACHE_MAX = _step_cache.CACHE_MAX
 
 
 # ---------------------------------------------------------------------------
@@ -456,8 +443,7 @@ def run_tui(
     )
     cons.print("[dim]  ,/. or ←/→  prev/next step  •  space play  •  l log  •  f freeze  •  ↑↓ variable  •  q quit[/dim]")
 
-    _cache.clear()
-    _cache[steps[0]] = first
+    _step_cache.seed(steps[0], first)
 
     app = MFCTuiApp(
         steps=steps,

toolchain/mfc/viz/viz.py

@@ -6,6 +6,7 @@
 
 import os
 import importlib
+import importlib.util
 import shutil
 import subprocess
 import sys
@@ -18,15 +19,13 @@
 def _ensure_viz_deps() -> None:
     """Install the [viz] optional extras on first use.
 
-    Checks for matplotlib as the sentinel package.  If it is missing the
-    whole viz extra is assumed to be absent and gets installed via uv (or pip
-    as a fallback) from the local toolchain directory.
+    Checks one sentinel per feature group so that a user who has matplotlib
+    pre-installed (e.g. via Anaconda) but lacks imageio, textual, or h5py
+    still triggers the install.
     """
-    try:
-        import matplotlib  # noqa: F401  # pylint: disable=import-outside-toplevel,unused-import
-        return  # already installed
-    except ImportError:
-        pass
+    _SENTINELS = ("matplotlib", "imageio", "h5py", "textual", "dash")
+    if all(importlib.util.find_spec(p) is not None for p in _SENTINELS):
+        return  # all present
 
     toolchain_path = os.path.join(MFC_ROOT_DIR, "toolchain")
     cons.print("[bold]Installing viz dependencies[/bold] "