diff --git a/statuspro_mcp_server/src/statuspro_mcp/tools/prefab_ui.py b/statuspro_mcp_server/src/statuspro_mcp/tools/prefab_ui.py index a1648ae..b1f5aa0 100644 --- a/statuspro_mcp_server/src/statuspro_mcp/tools/prefab_ui.py +++ b/statuspro_mcp_server/src/statuspro_mcp/tools/prefab_ui.py @@ -129,6 +129,24 @@ def _status_chip(code: str | None, name: str | None, color: str | None) -> None: } +def _preview_ref(field: str) -> Rx: + """Build a reactive reference to ``preview.`` in iframe state. + + The Confirm-button ``CallTool`` actions wire iframe state into tool + arguments via templates like ``{{ preview.order_id }}``. Bare strings + are fragile: a typo in the prefix (``{{ preivew.order_id }}``) silently + expands to empty and the host calls the tool with a missing arg. + + This helper doesn't validate the field at runtime — ``Rx`` accepts any + string. The win is structural: the ``preview.`` prefix exists in + exactly one place (this function body), so prefix typos at call sites + are impossible. Field-name typos like ``_preview_ref("ordr_id")`` are + still possible, but the field name is now a bare identifier that's + greppable and refactor-safe (vs. embedded in a template string). + """ + return Rx(f"preview.{field}") + + def _build_confirm_action( tool: str, arguments: dict[str, Any], @@ -512,12 +530,12 @@ def build_status_change_preview_ui( confirm_action=_build_confirm_action( "update_order_status", { - "order_id": "{{ preview.order_id }}", - "status_code": "{{ preview.new_status_code }}", - "comment": "{{ preview.comment }}", - "public": "{{ preview.public }}", - "email_customer": "{{ preview.email_customer }}", - "email_additional": "{{ preview.email_additional }}", + "order_id": _preview_ref("order_id"), + "status_code": _preview_ref("new_status_code"), + "comment": _preview_ref("comment"), + "public": _preview_ref("public"), + "email_customer": _preview_ref("email_customer"), + "email_additional": _preview_ref("email_additional"), "confirm": True, }, ), @@ -636,9 +654,9 @@ def build_comment_preview_ui(preview: dict[str, Any]) -> PrefabApp: confirm_action=_build_confirm_action( "add_order_comment", { - "order_id": "{{ preview.order_id }}", - "comment": "{{ preview.comment }}", - "public": "{{ preview.public }}", + "order_id": _preview_ref("order_id"), + "comment": _preview_ref("comment"), + "public": _preview_ref("public"), "confirm": True, }, ), @@ -689,9 +707,9 @@ def build_due_date_change_preview_ui(preview: dict[str, Any]) -> PrefabApp: confirm_action=_build_confirm_action( "update_order_due_date", { - "order_id": "{{ preview.order_id }}", - "due_date": "{{ preview.new_due_date }}", - "due_date_to": "{{ preview.new_due_date_to }}", + "order_id": _preview_ref("order_id"), + "due_date": _preview_ref("new_due_date"), + "due_date_to": _preview_ref("new_due_date_to"), "confirm": True, }, ), @@ -759,12 +777,12 @@ def build_bulk_status_change_preview_ui(preview: dict[str, Any]) -> PrefabApp: confirm_action=_build_confirm_action( "bulk_update_order_status", { - "order_ids": "{{ preview.order_ids }}", - "status_code": "{{ preview.target_status_code }}", - "comment": "{{ preview.comment }}", - "public": "{{ preview.public }}", - "email_customer": "{{ preview.email_customer }}", - "email_additional": "{{ preview.email_additional }}", + "order_ids": _preview_ref("order_ids"), + "status_code": _preview_ref("target_status_code"), + "comment": _preview_ref("comment"), + "public": _preview_ref("public"), + "email_customer": _preview_ref("email_customer"), + "email_additional": _preview_ref("email_additional"), "confirm": True, }, ), diff --git a/statuspro_mcp_server/tests/tools/test_prefab_ui.py b/statuspro_mcp_server/tests/tools/test_prefab_ui.py index c9ba5c0..b5b4660 100644 --- a/statuspro_mcp_server/tests/tools/test_prefab_ui.py +++ b/statuspro_mcp_server/tests/tools/test_prefab_ui.py @@ -95,6 +95,59 @@ def _find_tool_calls(envelope: dict[str, Any]) -> list[dict[str, Any]]: ] +# Smallest-viable preview fixtures for the four mutation cards. Used by tests +# that pin contracts across every preview card (state-machine guards, +# Cancel-no-SendMessage). Kept module-level so a new "every card must…" +# assertion can iterate over them without copying ~45 lines per test. +_PREVIEW_BUILDER_FIXTURES: list[tuple[Any, dict[str, Any]]] = [ + ( + build_status_change_preview_ui, + { + "order_id": 1, + "current_status_code": "st000002", + "new_status_code": "st000003", + "comment": None, + "public": False, + "email_customer": True, + "email_additional": False, + "valid": True, + "viable_status_codes": ["st000003"], + }, + ), + ( + build_comment_preview_ui, + { + "order_id": 1, + "order_summary": {"id": 1}, + "comment": "hi", + "public": False, + }, + ), + ( + build_due_date_change_preview_ui, + { + "order_id": 1, + "order_summary": {"id": 1}, + "current_due_date": "2026-01-01", + "new_due_date": "2026-02-01", + }, + ), + ( + build_bulk_status_change_preview_ui, + { + "order_ids": [1, 2], + "order_count": 2, + "target_status_code": "st000003", + "target_status_name": "Shipped", + "comment": None, + "public": False, + "email_customer": True, + "email_additional": False, + }, + ), +] + + def test_build_orders_table_ui_renders_with_drill_down_action(): app = build_orders_table_ui( [ @@ -397,54 +450,7 @@ def test_preview_cards_wire_double_click_guard(): is the spam guard; the explicit ``disabled={{ pending || cancelled }}`` binding on the button is belt-and-suspenders. """ - builders = [ - ( - build_status_change_preview_ui, - { - "order_id": 1, - "current_status_code": "st000002", - "new_status_code": "st000003", - "comment": None, - "public": False, - "email_customer": True, - "email_additional": False, - "valid": True, - "viable_status_codes": ["st000003"], - }, - ), - ( - build_comment_preview_ui, - { - "order_id": 1, - "order_summary": {"id": 1}, - "comment": "hi", - "public": False, - }, - ), - ( - build_due_date_change_preview_ui, - { - "order_id": 1, - "order_summary": {"id": 1}, - "current_due_date": "2026-01-01", - "new_due_date": "2026-02-01", - }, - ), - ( - build_bulk_status_change_preview_ui, - { - "order_ids": [1, 2], - "order_count": 2, - "target_status_code": "st000003", - "target_status_name": "Shipped", - "comment": None, - "public": False, - "email_customer": True, - "email_additional": False, - }, - ), - ] - for builder, preview in builders: + for builder, preview in _PREVIEW_BUILDER_FIXTURES: envelope = _envelope(builder(preview)) state = envelope.get("state") or {} # The four state slots the footer's If/Elif blocks bind to. @@ -563,6 +569,43 @@ def test_retry_button_has_double_click_guard(): ) +def test_cancel_buttons_do_not_send_chat_messages(): + """Cancel on every preview card must dismiss client-side (SetState + + optional toast) — never fire ``SendMessage``. + + SendMessage round-trips through the LLM and shows up as a fake user + message in the chat, which is noisy and unnecessary now that Confirm + fires the apply directly via CallTool (no agent middleman). Pinning + this so a future regression that re-introduces SendMessage on Cancel + surfaces here instead of as chat-noise in production. + """ + for builder, preview in _PREVIEW_BUILDER_FIXTURES: + envelope = _envelope(builder(preview)) + cancel_buttons = [ + n + for n in _walk_nodes(envelope) + if n.get("type") == "Button" and n.get("label") == "Cancel" + ] + assert cancel_buttons, f"{builder.__name__}: no Cancel button found in envelope" + for cancel in cancel_buttons: + # ``_build_cancel_action`` returns ``list[Action]``, so the + # serialized on_click is always a list — no defensive coercion + # needed. + on_click = cancel.get("onClick") or [] + assert isinstance(on_click, list), ( + f"{builder.__name__}: Cancel on_click must be a list; got {on_click!r}" + ) + send_messages = [ + a + for a in on_click + if isinstance(a, dict) and a.get("action") == "sendMessage" + ] + assert not send_messages, ( + f"{builder.__name__}: Cancel button must not fire SendMessage; " + f"got {send_messages}" + ) + + def test_status_change_preview_invalid_does_not_seed_apply_state(): """When ``valid=False`` the Confirm button is replaced with a "See viable transitions" button — the apply rail isn't live, so the pending/applied