Skip to content

feat(tools): configure_popup_modal_layer + slash-prompt for custom popup modal feature - #10

Merged
romer8 merged 2 commits into
mainfrom
feat/configure-popup-modal-layer
May 21, 2026
Merged

feat(tools): configure_popup_modal_layer + slash-prompt for custom popup modal feature#10
romer8 merged 2 commits into
mainfrom
feat/configure-popup-modal-layer

Conversation

@romer8

@romer8 romer8 commented May 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds an MCP surface for the custom popup-modal feature shipped on the tethysdash side 2026-05-13 (Aquaveo/tethysapp-tethys_dash#124). Before this PR the popup modal was UI-only — there was no MCP-tool path for the chatbox to configure it. The new tool emits a single-op {patch_update} envelope and re-uses the existing apply_patch dispatch path; no chatbox-core/engine changes.

Closes the surface gap diagnosed in /ce-debug 2026-05-21.

  • New @mcp.tool configure_popup_modal_layer(map_uuid, layer_index, popup_config) — emits {patch_update: {uuid, source: "Map", ops: [single add op at /args/layers/<N>/popupConfig]}}
  • New @mcp.prompt configure_popup_modal_layer slash-command counterpart
  • 3 Pydantic models (_PopupConfigPayload, _PopupConfigPosition, _PopupConfigGridItemInput) with extra="forbid" to catch LLM old-shape leaks
  • Server-side normalization mirrors PopupLayoutEditor.js's canonical persisted shape (mint uuid4/i/id: None, stringify argsargs_string, stringify metadatametadata_string)
  • Structured {error, fix_hint, errors[]} envelope on validation failure for one-cycle LLM repair
  • Tool catalog: 25 → 26

Design rationale

Settled in docs/plans/2026-05-21-002-feat-tethysdash-mcps-popup-modal-surface-plan.md (with full doc-review):

  • Dedicated tool, not popup_options extension on 11 add_*_layer tools — the popupConfig envelope is shape-invariant across source types; only one shared tool is needed. The t3-split lesson applies inside gridItems (per-source-type args), not at the envelope.
  • layer_index: int not layer_name: str — the standalone MCP server is stateless re: dashboard contents and can't resolve a name to an index. The LLM reads the index from dashboard_state (same pattern as patch_visualization).
  • Canonical persisted shape on the wire (not a "friendly" shape with server-side translation) — eliminates a translation layer; round-trip parity with the React UI is trivial.
  • Inline gridItems list, single-call — popups in practice carry 1-3 gridItems; multi-call composability deferred to v2.
  • ${feature.<key>} syntax named abstractly in field descriptions — no concrete example values per feedback_no_examples_in_tool_descriptions.md.

Tool description carries (load-bearing prose, pinned by tests)

  • Mutual-exclusion clause: "DO NOT use this tool to edit fields on an existing popupConfig — use patch_visualization on the specific sub-path instead. Re-calling this tool REPLACES the entire popupConfig."
  • Same-turn race constraint: "Configure the popup in a turn AFTER add_*_layer lands."
  • ${feature.<key>} syntax named (no concrete example values)
  • Pointers to all 3 discovery surfaces for gridItems[].source: list_available_visualizations, list_intake_plugins, register_runtime_plugin

Known v1 trade-offs (documented in plan)

  • Destructive replace on re-call — server mints fresh uuid4 per gridItem; any popup-internal cross-references (e.g., variable_input bindings nested inside the popup) silently break. Mutual-exclusion clause in description is the v1 mitigation. v2: optional replace: true guard if observed in smoke.
  • Engine rejectedPatches categorization at lib/chatbox-core/engine/index.js:1037-1047 is name-keyed to toolName === 'patch_visualization'. The new tool's {error} envelopes reach the LLM repair loop but do NOT populate the user-facing _buildWhitelistWarning banner. Accepted v1 gap.
  • No per-source-type args validation — runtime errors on bad args shapes surface via the existing per-tile error boundary. v2: strict validation against the registered viz type's schema if observed failure rate justifies.
  • Cheap registry-membership check on gridItems[].source was planned (KTD feat: dev runbook + setup-mcp.sh for running alongside tethysdash #2) but dropped during implementation — implementing it properly requires a network call to list_intake_plugins (HTTP-backed), which contradicts the stateless-server premise. Deferred to v2.

Tests

51 new tests in test_mcp/test_popup_modal_layer.py:

  • TestPydanticModels (13 tests) — shape validation: minimal payload, full payload, position range, gridItem ranges, empty gridItems rejected, mode literal, extra-field rejection
  • TestToolHappyPath (12 tests) — patch_update envelope shape, single-op atomicity, layer_index path interpolation, JSON-string popup_config coercion, default position/metadata/titleTemplate, ${feature.*} + ${variable_name} preservation through json.dumps
  • TestToolErrorPaths (7 tests) — invalid uuid, empty gridItems, mode='carousel', popup_config=None / "" / non-dict, negative layer_index
  • TestEnvelopeContract (7 tests) — canonical persisted gridItem fields, no LLM-input-shape leak, args_string/metadata_string are valid JSON, uuid is str(uuid4), id is None, re-call mints fresh UUIDs
  • TestWhitelistCoverage (5 tests) — /args/layers prefix admits all deeper popupConfig paths
  • TestSlashPrompt (2 tests) — prompt scaffolds the tool call
  • TestToolDescription (5 tests) — regression guards for mutual-exclusion + same-turn-race + abstract ${feature.<key>} + discovery-tool pointers + "replaces" overwrite semantic

Full suite: 890 passed, zero regressions.

Test plan

  • pytest test_mcp/ --no-cov -q → 890 passed
  • Pydantic models compile + import cleanly
  • FastMCP registers the new tool + prompt (catalog grows 25 → 26)
  • Manual smoke (Unit 6 of plan, deferred to follow-up): GEOGLOWS Demo prompt from /ce-debug 2026-05-21 against a real LLM, with no manual UI fallback step
  • Tethysdash-side Unit 5 (preview UX fix for findUnresolvedFeatureTokens guard in the Edit Visualization sub-modal): deferred to separate PR in tethysapp-tethys_dash on feature/tethysdash-mcp-server

Plan

docs/plans/2026-05-21-002-feat-tethysdash-mcps-popup-modal-surface-plan.md

romer8 added 2 commits May 21, 2026 16:24
Adds an MCP surface for the custom popup-modal feature shipped on the
tethysdash side 2026-05-13 (Aquaveo/tethysapp-tethys_dash#124). The new
tool emits a single-op {patch_update} envelope with an RFC 6902 `add` op
at `/args/layers/<layer_index>/popupConfig`, re-using the existing
apply_patch dispatch path in chatbox-core + DashboardLayout — no engine
wiring change required.

Surface:
- New @mcp.tool `configure_popup_modal_layer(map_uuid, layer_index, popup_config)`
- New @mcp.prompt `configure_popup_modal_layer` (slash-command counterpart)
- 3 Pydantic models (_PopupConfigPayload, _PopupConfigPosition,
  _PopupConfigGridItemInput) with extra="forbid" to catch LLM old-shape
  leaks like `visualizationType` / `props`
- `_popup_validation_error_envelope` converts Pydantic ValidationError to
  the structured {error, fix_hint, errors[]} envelope for one-cycle LLM
  repair

Server-side normalization mirrors PopupLayoutEditor.js's canonical
persisted shape — mint uuid4/i/id, stringify args→args_string and
metadata→metadata_string — so round-trip parity with UI-authored popups
is trivial (no React-side renormalization needed). ${feature.<key>} and
${variable_name} template strings inside titleTemplate + gridItem args
are preserved verbatim; substitution happens at render time inside the
popup's FeatureScopedVariableInputs / VariableInputsContext scope.

Tool description carries:
- Mutual-exclusion clause (full-overwrite tool; partial edits go through
  patch_visualization per /args/layers/N/popupConfig/<subpath>) per
  feedback_create_patch_mutual_exclusion.md
- Same-turn race constraint (configure popups in a turn AFTER add_*_layer
  — dashboard_state snapshot is stale within the same turn)
- ${feature.<key>} syntax named abstractly (no concrete example values
  per feedback_no_examples_in_tool_descriptions.md)
- Pointer to discovery tools (list_available_visualizations,
  list_intake_plugins, register_runtime_plugin) for valid gridItem source
  names across all three registries

Tests: test_popup_modal_layer.py — 51 new tests across Pydantic shape
validation, tool happy/error paths, envelope canonical-shape contract,
whitelist coverage (proves /args/layers prefix already admits all deeper
popupConfig sub-paths), slash-prompt scaffolding, and tool-description
clause regression guards. Full suite: 890 tests pass.

Tool catalog count: 25 → 26 (README + mcp_server.py prose updated;
CHANGELOG entries for prior smoke-test runs left as historical record).

Plan: docs/plans/2026-05-21-002-feat-tethysdash-mcps-popup-modal-surface-plan.md
…e first-time-setup framing

Debug session 2026-05-21 turn 2 with gemini-flash: asked to 'Enable
the Custom Popup Modal on the China Flowlines layer...' (a first-time
popup-modal setup), the LLM routed to patch_visualization instead of
configure_popup_modal_layer, then fabricated:
- wrong path: /args/layers/0/configuration/props/popup
  (canonical is /args/layers/0/popupConfig)
- wrong value shape: {content, title, type} instead of
  {mode, position, titleTemplate, gridItems}

Two reasons the LLM picked the wrong tool:

1. The system prompt's PRIORITY clause sends 'modify existing' →
   patch_visualization. 'Enable popup on existing layer' reads as
   modify-existing. (System-prompt fix is a sibling PR in tethysdash:
   add a SECOND exception to the PRIORITY clause for
   configure_popup_modal_layer.)

2. configure_popup_modal_layer's description LED with the exclusion
   clause ('DO NOT use this tool to edit fields on an existing
   popupConfig — use patch_visualization on the specific sub-path'),
   which the LLM appears to have read as 'don't use this tool for
   anything that touches an existing layer.'

This commit fixes #2 by restructuring the description to lead with
positive use:

  'USE THIS TOOL for FIRST-TIME popup-modal setup on a map layer. It
   is THE correct tool whenever the user asks to enable, add, configure,
   create, or set up a Custom Popup Modal on an existing map layer —
   even though the map layer itself already exists. Adding a popupConfig
   to a layer for the first time is NOT modifying an existing
   visualization in the patch_visualization sense; it's a structured
   setup operation with its own tool.'

Plus an explicit anti-pattern call-out:

  'DO NOT use patch_visualization to add a popupConfig from scratch —
   the popupConfig shape is non-trivial (mode, position, titleTemplate,
   gridItems with source/args/x/y/w/h), the canonical path is
   /args/layers/N/popupConfig (not /args/layers/N/configuration/...),
   and this tool builds the persisted shape correctly.'

The patch_visualization carve-out is preserved but scoped to its real
case: partial edits to an already-existing popupConfig.

Tests: split the prior test_description_names_mutual_exclusion into two
assertions — one for positive-use leading (test_description_leads_with_
positive_use), one for the partial-edits-only carve-out
(test_description_names_patch_visualization_carveout). Other 50 tests
in TestPopupModalLayer / TestToolHappyPath / TestToolErrorPaths /
TestEnvelopeContract / TestWhitelistCoverage / TestSlashPrompt /
TestToolDescription still pass.

Full suite: 891 passed.
@romer8
romer8 force-pushed the feat/configure-popup-modal-layer branch from bcc1dd0 to bf372d1 Compare May 21, 2026 22:24
@romer8
romer8 merged commit 4457fe6 into main May 21, 2026
2 checks passed
@romer8
romer8 deleted the feat/configure-popup-modal-layer branch May 21, 2026 22:25
romer8 added a commit that referenced this pull request May 22, 2026
Eight PRs from the 2026-05-21 popup-modal arc, grouped into Added /
Changed / Fixed / Tests / Documentation per Keep a Changelog. New tool
+ slash-prompt for configure_popup_modal_layer (PR #10), case-fold
args normalization for popup gridItems + render_plugin +
add_dynamic_map_layer (PRs #14 + #15), ESRI sublayer-ID extraction
from LAYERDEFS + ESRI Image guard widening (PRs #12 + #17), WMS
attr_key from wms_layers (PR #16), popup_options outer-key
normalization across all 11 add_*_layer tools (PR #11), tool
description tightening for case-sensitive arg_names + 100-col grid
(PR #13). Suite grew 930 → 961.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant