fix(mcp): repair DataTable onRowClick per-row binding + Slot/RESULT envelope

#494 asked whether the DataTable ``onRowClick`` per-row ``{{ field }}``
substitution actually works end-to-end through the iframe. It does NOT,
and the new browser tests prove it:

- ``{{ sku }}`` and ``{{ id }}`` are NOT resolved — the renderer's
  action-scope spreads the clicked row dict at ``$event``, not into the
  scope's top level. Only ``$event``, ``$result``, ``$error``, and
  state keys are direct lookups. The renderer leaves unresolved
  templates as literal strings, so the host calls ``get_variant_details``
  with ``sku="{{ sku }}"`` verbatim and the tool either rejects the
  call or treats it as a missing-SKU lookup.

- Even with the substitution fixed, the drill-down still doesn't render
  because ``SetState("detail", RESULT)`` puts the full PrefabApp
  envelope (``{$prefab, view, defs, state}``) into ``state.detail``,
  but ``Slot`` checks ``"type" in D`` on the stored value to decide
  whether to render — and the envelope's keys don't include ``type``.
  Slot falls back to its placeholder children every time.

Both failure modes are wired together: even if you fix the
substitution, the response card silently doesn't render. Both fixes
are needed, applied in the same commit so the drill-down ships working
end-to-end.

Fix shape
=========

1. ``{{ sku }}`` → ``str(EVENT.sku)`` (compiles to ``{{ $event.sku }}``).
   Same for ``{{ id }}`` → ``str(EVENT.id)``. The ``EVENT`` Rx exposes
   ``$event`` directly, and ``.sku`` / ``.id`` chains via dot-path to
   the row dict's fields. Applied to both row-click DataTables:
   ``build_search_results_ui`` and ``build_item_detail_ui``'s nested
   variants table.

2. ``on_success=SetState("detail", RESULT)`` →
   ``on_success=SetState("detail", RESULT.view)``. The ``RESULT`` Rx
   compiles to ``{{ $result }}`` which evaluates to the full envelope;
   ``RESULT.view`` evaluates to the root view component, which DOES
   have ``type`` and renders cleanly inside the Slot.

Verification
============

Three new browser tests in ``test_other_cards_render.py``:

- ``test_search_results_row_click_passes_clicked_sku`` — clicks row
  ``SKU-0003``, asserts the stub received ``received_sku="SKU-0003"``
  (not the literal template, not None, not some other row's SKU).
  Cross-process verification via a temp-file the stub writes to.
- ``test_item_detail_variant_row_click_passes_clicked_variant_id`` —
  same shape for the item-detail variants table, asserts
  ``received_variant_id=700002`` (the middle row's id).
- ``test_search_results_row_click_populates_detail_slot`` — full
  end-to-end DOM assertion: after click, the echoed response card
  ("Echoed Variant Details" + the SKU) actually renders inside the
  detail Slot.

The substitution-side test ALONE is the diagnostic that catches a
host-side regression in #491-class binding handling. Pre-fix it
caught the literal-template bug; post-fix it pins the working
behavior. The Slot-render test pins the envelope-extraction fix.

Empirical note: the renderer attaches the row-click handler at the
``<td>`` cell level, not the ``<tr>`` row level — ``tr.click()`` and
``tr.dispatchEvent("click")`` both fail to fire the handler, while
``td.click()`` does. The tests use the cell selector as the
canonical click target.

Closes #494.

Author: dougborg

katana_mcp_server/src/katana_mcp/tools/prefab_ui.py

@@ -75,7 +75,7 @@
 )
 from prefab_ui.components.control_flow import Elif, Else, If
 from prefab_ui.components.slot import Slot
-from prefab_ui.rx import RESULT, Rx
+from prefab_ui.rx import EVENT, RESULT, Rx
 from pydantic import BaseModel
 
 from katana_mcp.tools.tool_result_utils import BLOCK_WARNING_PREFIX
@@ -526,18 +526,25 @@ def build_search_results_ui(
             search=True,
             paginated=True,
             pageSize=20,
-            # NOTE: ``{{ sku }}`` and ``{{ $error }}`` here are *per-row /
-            # event-context* bindings provided by the DataTable component
-            # itself, NOT the iframe-state substitution that broke in #491.
-            # The DataTable renderer expands these client-side from the
-            # clicked row's data and the action's error payload, so they
-            # do not depend on the host-side Mustache-from-state mechanism
-            # that silently dropped args. Reliability is owned by the
-            # DataTable component; verification is tracked in #494.
+            # Per-row binding via ``$event``: the renderer spreads the
+            # clicked row's dict into the action-scope as ``$event``, so
+            # ``{{ $event.sku }}`` resolves to the row's SKU. The naive
+            # form ``{{ sku }}`` does NOT work — only top-level scope keys
+            # ($event, $result, $error, state keys) are direct lookups;
+            # the row dict itself is not spread (#494, verified by browser
+            # test ``test_search_results_row_click_passes_clicked_sku``).
+            # ``{{ $error }}`` works because $error IS a top-level key.
+            #
+            # ``RESULT.view`` (not bare ``RESULT``) on the success path:
+            # the tool returns a full PrefabApp envelope
+            # (``{$prefab, view, defs, state}``) and the Slot renderer
+            # expects a single component dict with a top-level ``type``.
+            # Setting ``state.detail = $result.view`` extracts the
+            # root view component so the Slot can render it (#494).
             onRowClick=CallTool(
                 "get_variant_details",
-                arguments={"sku": "{{ sku }}"},
-                on_success=SetState("detail", RESULT),
+                arguments={"sku": str(EVENT.sku)},
+                on_success=SetState("detail", RESULT.view),
                 on_error=ShowToast("{{ $error }}", variant="error"),
             ),
         )
@@ -963,18 +970,18 @@ def _item_variants_table(item: dict[str, Any]) -> None:
         # Per-row click invokes get_variant_details using the row's
         # variant id, not its SKU. ``ItemVariantSummary.sku`` is
         # nullable (Katana allows variants without a SKU on the wire),
-        # so a click on a SKU-less row would otherwise produce
-        # ``arguments={"sku": null}`` and the tool would reject the
-        # call with "must provide at least one of: sku, variant_id,
-        # skus, variant_ids". ``id`` is always present on the
+        # so binding by SKU would reject every SKU-less row with the
+        # tool's "must provide at least one of: sku, variant_id, skus,
+        # variant_ids" error. ``id`` is always present on the
         # ``ItemVariantSummary`` shape, so this path stays clickable
-        # for every row. DataTable's per-row ``{{ id }}`` binding
-        # expands client-side from row data (not iframe state) so it
-        # doesn't hit the #491 state-binding failure mode.
+        # for every row. Per-row substitution uses ``EVENT.id`` (which
+        # compiles to ``{{ $event.id }}``) because the row dict isn't
+        # spread into scope — see the comment on the search_results
+        # DataTable for the full reasoning (#494).
         onRowClick=CallTool(
             "get_variant_details",
-            arguments={"variant_id": "{{ id }}"},
-            on_success=SetState("detail", RESULT),
+            arguments={"variant_id": str(EVENT.id)},
+            on_success=SetState("detail", RESULT.view),
             on_error=ShowToast("{{ $error }}", variant="error"),
         ),
     )

katana_mcp_server/tests/browser/render_test_server.py

@@ -12,7 +12,10 @@
 
 from __future__ import annotations
 
+import json
+import tempfile
 from collections.abc import Callable
+from pathlib import Path
 from typing import Any
 
 from fastmcp import FastMCP
@@ -25,6 +28,7 @@
 from katana_mcp.tools.prefab_ui import (
     build_batch_recipe_update_ui,
     build_inventory_check_ui,
+    build_item_detail_ui,
     build_modification_preview_ui,
     build_modification_result_ui,
     build_search_results_ui,
@@ -284,6 +288,42 @@ def _verification_app() -> PrefabApp:
     return build_verification_ui(response)
 
 
+def _item_detail_app() -> PrefabApp:
+    """build_item_detail_ui with 3 variants — exercises the variants
+    DataTable's per-row ``onRowClick=CallTool(get_variant_details,
+    arguments={"variant_id": "{{ id }}"})`` binding (#494).
+    """
+    item = {
+        "id": 9001,
+        "name": "Test Product",
+        "type": "product",
+        "uom": "pcs",
+        "is_sellable": True,
+        "is_producible": True,
+        "variants": [
+            {
+                "id": 700001,
+                "sku": "VAR-A",
+                "sales_price": 10.00,
+                "purchase_price": 4.00,
+            },
+            {
+                "id": 700002,
+                "sku": "VAR-B",
+                "sales_price": 20.00,
+                "purchase_price": 8.00,
+            },
+            {
+                "id": 700003,
+                "sku": "VAR-C",
+                "sales_price": 30.00,
+                "purchase_price": 12.00,
+            },
+        ],
+    }
+    return build_item_detail_ui(item)
+
+
 def _batch_recipe_update_app() -> PrefabApp:
     """build_batch_recipe_update_ui with 5 sub-ops — exercises rows='{{ rows }}'."""
     response = {
@@ -405,6 +445,7 @@ def _stock_adjustment_delete_response(*, is_preview: bool) -> dict:
     # Audit coverage: post-mustache-fix render checks for every other
     # state-bound DataTable card.
     "search_results": _search_results_app,
+    "item_detail": _item_detail_app,
     "inventory_check": _inventory_check_app,
     "verification": _verification_app,
     "batch_recipe_update": _batch_recipe_update_app,
@@ -537,5 +578,60 @@ async def modify_manufacturing_order(
     return make_tool_result(response, ui=ui)
 
 
+class _EchoVariantDetails(BaseModel):
+    """Echoes back the arguments the host actually passed to
+    ``get_variant_details`` — used by the #494 row-click tests to verify
+    DataTable per-row ``{{ field }}`` substitution resolved against the
+    clicked row's data (not null, not the literal Mustache string).
+    """
+
+    received_sku: str | None = None
+    received_variant_id: int | None = None
+
+
+# Fixed cross-process path so the browser-test subprocess and the
+# pytest process agree on where the stub writes its received args.
+# Cleared before each row-click test and read back after the click.
+GET_VARIANT_DETAILS_RECORD_PATH = (
+    Path(tempfile.gettempdir()) / "katana_test_get_variant_details_received.json"
+)
+
+
+@mcp.tool(meta={"ui": True})
+async def get_variant_details(
+    sku: str | None = None,
+    variant_id: int | None = None,
+) -> ToolResult:
+    """Echo stub for #494 row-click binding tests.
+
+    The two row-click drill-downs that exercise per-row binding:
+
+    - ``build_search_results_ui``: ``arguments={"sku": "{{ sku }}"}``
+    - ``build_item_detail_ui`` variants table: ``arguments={"variant_id":
+      "{{ id }}"}``
+
+    A correctly-substituting host calls this with the clicked row's
+    actual value. A broken host calls it with ``None`` (silent drop), the
+    literal string ``"{{ sku }}"``, or fires ``on_error``. The stub
+    records whichever value the host actually delivered to a cross-process
+    file at :data:`GET_VARIANT_DETAILS_RECORD_PATH` so the browser test
+    can read it back after the click — the response card itself can't
+    be used as the assertion target because the Slot/RESULT envelope
+    mismatch (separately filed) prevents the response from rendering in
+    the ``detail`` slot.
+    """
+    received = {"received_sku": sku, "received_variant_id": variant_id}
+    GET_VARIANT_DETAILS_RECORD_PATH.write_text(json.dumps(received))
+    response = _EchoVariantDetails(received_sku=sku, received_variant_id=variant_id)
+    from prefab_ui.components import Card, CardContent, CardTitle, Text
+
+    with PrefabApp(state={}, css_class="p-4") as ui, Card():
+        CardTitle(content="Echoed Variant Details")
+        with CardContent():
+            Text(content=f"received_sku={sku!r}")
+            Text(content=f"received_variant_id={variant_id!r}")
+    return make_tool_result(response, ui=ui)
+
+
 if __name__ == "__main__":
     mcp.run(transport="http", port=8765)

katana_mcp_server/tests/browser/test_other_cards_render.py

@@ -15,10 +15,23 @@
 
 from __future__ import annotations
 
+import json
+import tempfile
+import time
+from pathlib import Path
+
 import pytest
 
 pytestmark = pytest.mark.browser
 
+# Pinned to match ``render_test_server.GET_VARIANT_DETAILS_RECORD_PATH``
+# so the test process and the dev-server subprocess agree on where the
+# stub writes its received args. If the constant moves, both sides need
+# to update.
+_GET_VARIANT_DETAILS_RECORD_PATH = (
+    Path(tempfile.gettempdir()) / "katana_test_get_variant_details_received.json"
+)
+
 
 class TestOtherCardsRender:
     """Verify state-bound DataTables outside the modification surface
@@ -84,3 +97,137 @@ def test_batch_recipe_update_renders(self, render_scenario):
         assert frame.locator("table tr").count() >= 6
         # SKU strings from the test fixture.
         assert frame.locator("text=SKU-OLD-0").count() >= 1
+
+
+class TestDataTableRowClickBinding:
+    """Verify DataTable ``onRowClick`` ``{{ field }}`` per-row bindings
+    actually resolve client-side against the clicked row's data (#494).
+
+    Background: #491 found that the MCP host silently dropped Mustache
+    ``{{ request.<field> }}`` arguments in CallTool actions (host-state
+    substitution). The fix in #493 inlined values at preview-build time
+    for the order/receipt/batch/fulfill builders, but deliberately left
+    the DataTable per-row ``{{ sku }}`` and ``{{ id }}`` bindings alone
+    because those expand via the DataTable component's own row-context
+    machinery (``$event`` is the row dict per the component docstring)
+    — a different code path. These tests prove that path actually works
+    end-to-end through the iframe.
+
+    Both card builders' row-click handlers route to a stub
+    ``get_variant_details`` (defined in ``render_test_server.py``) that
+    echoes back whichever argument the host actually delivered. The
+    on_success ``SetState("detail", RESULT)`` then renders the echoed
+    card in a ``Slot(name="detail")``. The test asserts the echoed text
+    matches the clicked row's data — proving substitution worked.
+
+    Failure modes the tests catch:
+    - Host drops the binding silently → echo shows ``None`` → fail
+    - Host emits the literal Mustache string → echo shows ``"{{ sku }}"``
+      → fail
+    - on_error fires (host rejects the call upfront) → toast appears
+      but the detail slot never populates → fail (wait_for times out)
+    """
+
+    def _wait_and_read_record(self, timeout_s: float = 15.0) -> dict:
+        """Poll the cross-process record file until the stub writes to it,
+        then parse + return its contents."""
+        deadline = time.monotonic() + timeout_s
+        while time.monotonic() < deadline:
+            if _GET_VARIANT_DETAILS_RECORD_PATH.exists():
+                return json.loads(_GET_VARIANT_DETAILS_RECORD_PATH.read_text())
+            time.sleep(0.25)
+        raise AssertionError(
+            f"Stub never wrote to {_GET_VARIANT_DETAILS_RECORD_PATH} within "
+            f"{timeout_s}s — the row-click never reached the server. Either "
+            "the host dropped the CallTool, or the click target didn't fire "
+            "onRowClick."
+        )
+
+    @pytest.fixture(autouse=True)
+    def _clear_record(self):
+        """Wipe the cross-process record file before each test so a stale
+        write from a prior test can't masquerade as a fresh substitution."""
+        _GET_VARIANT_DETAILS_RECORD_PATH.unlink(missing_ok=True)
+        yield
+        _GET_VARIANT_DETAILS_RECORD_PATH.unlink(missing_ok=True)
+
+    def test_search_results_row_click_passes_clicked_sku(self, render_scenario):
+        """``build_search_results_ui`` DataTable: clicking row N fires
+        ``get_variant_details(sku="SKU-NNNN")`` with the row's own SKU.
+
+        Picks "SKU-0003" — far enough from the first row that a
+        first-row-only binding would also be caught. Clicks a ``<td>``
+        cell within the row because the Prefab DataTable renderer
+        currently attaches the click listener at the cell level (verified
+        empirically — ``tr.click()`` and ``tr.dispatchEvent("click")``
+        do not fire the handler, but a click on a contained ``<td>``
+        does).
+        """
+        frame = render_scenario("search_results")
+        # Click the SKU cell within the row whose SKU is "SKU-0003".
+        frame.locator("td.pf-table-cell").filter(has_text="SKU-0003").first.click()
+
+        record = self._wait_and_read_record()
+        # Substitution must have resolved the {{ sku }} binding against
+        # the clicked row's data — not None, not the literal Mustache
+        # string, not some other row's SKU.
+        assert record["received_sku"] == "SKU-0003", (
+            f"Expected received_sku='SKU-0003' (the clicked row's SKU). "
+            f"Got: {record!r}. If 'received_sku' is None the host silently "
+            f"dropped the binding. If it's '{{ sku }}' the host emitted the "
+            f"literal template. Either is the #491-class failure mode."
+        )
+        # search_results binds only {{ sku }}, not {{ id }} — variant_id
+        # should be absent.
+        assert record["received_variant_id"] is None
+
+    def test_item_detail_variant_row_click_passes_clicked_variant_id(
+        self, render_scenario
+    ):
+        """``build_item_detail_ui`` variants DataTable: clicking variant
+        row N fires ``get_variant_details(variant_id=N)`` with the row's
+        own id. Pins the #494 fix path for the card landed in #698.
+
+        Picks variant_id=700002 (the middle row) so first-row-only and
+        last-row-only bugs would both fail this test. The middle row's
+        SKU is "VAR-B" — click that cell to drive the row-click.
+        """
+        frame = render_scenario("item_detail")
+        frame.locator("td.pf-table-cell").filter(has_text="VAR-B").first.click()
+
+        record = self._wait_and_read_record()
+        assert record["received_variant_id"] == 700002, (
+            f"Expected received_variant_id=700002 (the clicked row's id). "
+            f"Got: {record!r}. The item_detail variants DataTable binds "
+            f"{{ id }} via on_row_click — a None or wrong-row value here "
+            f"means the row-context substitution is broken."
+        )
+        # item_detail binds only {{ id }}, not {{ sku }}.
+        assert record["received_sku"] is None
+
+    def test_search_results_row_click_populates_detail_slot(self, render_scenario):
+        """End-to-end drill-down: clicking a row not only fires the
+        CallTool with the right SKU, but the returned card actually
+        renders inside the ``Slot(name="detail")``.
+
+        The Slot-side fix paired with the substitution fix: a bare
+        ``SetState("detail", RESULT)`` would put the PrefabApp envelope
+        (``{$prefab, view, defs, state}``) into state, which the Slot
+        can't render because it checks ``"type" in D`` on the stored
+        value. ``RESULT.view`` (``{{ $result.view }}``) extracts the
+        root view component, which DOES have ``type``, so the Slot
+        renders the response card.
+        """
+        frame = render_scenario("search_results")
+        # Pre-click: fallback content is visible; echoed card is not.
+        assert frame.locator("text=Click a row to see").count() >= 1
+        assert frame.locator("text=Echoed Variant Details").count() == 0
+
+        frame.locator("td.pf-table-cell").filter(has_text="SKU-0007").first.click()
+
+        # The echoed response card appears in the detail slot.
+        frame.locator("text=Echoed Variant Details").wait_for(
+            state="visible", timeout=15000
+        )
+        # And it shows the clicked row's SKU echoed back.
+        assert frame.locator("text=received_sku='SKU-0007'").count() >= 1

katana_mcp_server/tests/test_prefab_ui.py

@@ -773,9 +773,12 @@ def test_variants_render_as_datatable_with_callTool_drilldown(self):
         assert on_row_click.get("tool") == "get_variant_details"
         # Argument keys off the row's ``id`` field, not ``sku``. Using
         # ``id`` keeps SKU-less variants (legitimate per
-        # ``ItemVariantSummary.sku: str | None``) clickable.
+        # ``ItemVariantSummary.sku: str | None``) clickable. The template
+        # uses ``$event.id`` (not bare ``id``) because the renderer
+        # spreads the row dict at ``$event``, not into the scope's top
+        # level (#494, verified via browser test).
         args = on_row_click.get("arguments") or {}
-        assert args.get("variant_id") == "{{ id }}"
+        assert args.get("variant_id") == "{{ $event.id }}"
         assert "sku" not in args
 
     def test_variants_table_handles_sku_less_variants(self):
@@ -805,7 +808,7 @@ def test_variants_table_handles_sku_less_variants(self):
         assert len(tables) == 1
         # Row-click args reference the variant id, not sku.
         args = tables[0].get("onRowClick", {}).get("arguments") or {}
-        assert args == {"variant_id": "{{ id }}"}
+        assert args == {"variant_id": "{{ $event.id }}"}
         # The variant count metric reflects the single row.
         assert "Variants: 1" in str(envelope)