From e5906c2f0e66e32a215d75e351cd4775bad9c82d Mon Sep 17 00:00:00 2001 From: Tienson Qin Date: Mon, 19 Oct 2026 13:31:11 +0800 Subject: [PATCH] fix(ai): preserve host rendering and metadata in grouped tools --- ai/transcript.ml | 25 ++-- ai/transcript.mli | 8 +- .../2026-10-19-grouped-tool-host-renderer.md | 71 ++++++++++++ test/test_lui_ai.ml | 109 ++++++++++++++++++ 4 files changed, 204 insertions(+), 9 deletions(-) create mode 100644 docs/agent-guide/proposed/bugfix/2026-10-19-grouped-tool-host-renderer.md diff --git a/ai/transcript.ml b/ai/transcript.ml index ea620066..cc8b34f7 100644 --- a/ai/transcript.ml +++ b/ai/transcript.ml @@ -230,6 +230,20 @@ let tool_card cfg (item_s : item Signal.signal) : t = ~accessibility_identifier:"lui-ai-tool" [ tool_detail cfg item_s ]) +let rendered_body cfg fallback item_s = + match cfg.render_item item_s with + | Some view -> view + | None -> fallback item_s + +let grouped_item cfg (item_s : item Signal.signal) : t = + scoped (fun _context -> + let item = sample item_s in + column ~key:item.id ~cross:`stretch + ~accessibility_identifier:("lui-ai-item:" ^ item.id) + ?style_class:(match cfg.item_class item with "" -> None | value -> Some value) + ~data_attrs:(reactive cfg.item_data_attrs item_s) + [ rendered_body cfg (tool_card cfg) item_s ]) + (* Collapsible section around a run of Tool_call items. Auto-expands while any member is active; user toggles persist in row scope. *) let tool_group cfg (row_s : row Signal.signal) : t = @@ -278,7 +292,7 @@ let tool_group cfg (row_s : row Signal.signal) : t = ~accessibility_identifier:"lui-ai-tool-group" [ column ~cross:`stretch ~gap:6 [ keyed ~source:items_s ~key:(fun (i : item) -> i.id) - ~cmp:String.compare ~mount:(tool_card cfg) ] ]) + ~cmp:String.compare ~mount:(grouped_item cfg) ] ]) let builtin_body cfg context (item_s : item Signal.signal) : t = ignore context; @@ -295,17 +309,12 @@ let builtin_body cfg context (item_s : item Signal.signal) : t = let item_row_of cfg context (item_s : item Signal.signal) : t = let item = sample item_s in - let body = - match cfg.render_item item_s with - | Some view -> view - | None -> builtin_body cfg context item_s - in + let body = rendered_body cfg (builtin_body cfg context) item_s in list_item ~key:item.id ~separator:`hidden ~accessibility_identifier:("lui-ai-item:" ^ item.id) ?style_class: (match cfg.item_class item with "" -> None | s -> Some s) - ?data_attrs: - (match cfg.item_data_attrs item with [] -> None | l -> Some l) + ~data_attrs:(reactive cfg.item_data_attrs item_s) [ row ~main:`center ~grow:1.0 [ column ~grow:1.0 ~max_width:cfg.content_width ~cross:`stretch ~padding_horizontal:8 ~padding_vertical:4 [ body ] ] ] diff --git a/ai/transcript.mli b/ai/transcript.mli index 6da350cf..fd7e8480 100644 --- a/ai/transcript.mli +++ b/ai/transcript.mli @@ -63,6 +63,10 @@ val transcript : steps" / "Worked through N steps" section ([group_tools], default [true]); the section auto-expands while any call is active and stays user-toggleable, keyed by the first member's id. + - [render_item] is consulted for both single items and grouped tool + children. Returning [None] uses the built-in body. Group children keep + their item identity and use non-interactive containers, so host tool + accordions and controls are not nested inside list-item buttons. - [on_retry] adds a resend control on [`uncertain] items (labelled [retry_label], default ["Retry same message"]); [item_actions] mounts host actions per row (copy, etc.) and receives the borrowed @@ -74,7 +78,9 @@ val transcript : visible (history paging). - [container_class]/[container_data_attrs] land on the scroll container itself (list or virtual_list) for host CSS/locators; - [item_class]/[item_data_attrs] land on each row. *) + [item_class]/[item_data_attrs] land on each item, including grouped + children. Data attributes update from the current item signal without + remounting its body. *) val item_row : Model.item -> t (** A single message row mounted outside [transcript] (previews). *) diff --git a/docs/agent-guide/proposed/bugfix/2026-10-19-grouped-tool-host-renderer.md b/docs/agent-guide/proposed/bugfix/2026-10-19-grouped-tool-host-renderer.md new file mode 100644 index 00000000..fb8b8cec --- /dev/null +++ b/docs/agent-guide/proposed/bugfix/2026-10-19-grouped-tool-host-renderer.md @@ -0,0 +1,71 @@ +# Grouped tool host renderer + +## Problem + +Transcript single rows use the host item renderer, metadata and styles, but +grouped tool children call the built-in tool card directly. Enabling work +grouping therefore loses host-specific tool details and identity attributes. + +## Proposal + +Route grouped children through a shared host-aware tool rendering helper while +retaining the built-in card when the host returns None. Preserve keyed child +identity, borrowed signal ownership, live metadata and the group disclosure +state. Use an ordinary non-interactive wrapper so tool accordions do not become +nested buttons. Keep the public API stable and update its documentation. + +This is an upstream LUI change, published as a PR before Mapre changes its +installed package pin. Do not copy library source into the consumer. The user's +clean composer and full conversation scrolling milestones are now deployed; +this repair resumes the remaining grouped activity work. + +## Alternatives considered + +### Replace the consumer transcript + +Rejected because it duplicates the library's grouping and lifecycle ownership +and conflicts with the requested reusable LUI AI integration. + +## Acceptance criteria + +- Actual mounted component tests reproduce missing host rendering before the + fix. Cover custom rendering, default fallback, updated tool content and + metadata, keyed child retention, disclosure and disposal without disposing + caller-owned signals. Keep pure grouping/order tests intact. +- Compile native and Melange targets and run the existing relevant test suites. + Validate actual browser behavior through the consumer after installation. +- Keep source functions at most 64 formatted lines. Do not modify Dune files, + disable warnings or use unsafe casts. Commit and push, then create the LUI PR. +- Update Mapre only after upstream review/acceptance and prove its tool details, + identities, grouped summaries and scroll behavior in an actual browser. + +## Risks + +- Grouping can remount children or accidentally dispose their borrowed signals. + A native protocol-tree check alone does not establish browser semantics. + +## Questions + +None. The user explicitly requests grouped work in the UI reference, reuse of +LUI components, an upstream PR first and no vendoring. + +## Verification + +Four mounted component regressions fail against the original implementation: +grouped host rendering, grouped live metadata/callbacks, ordinary live metadata, +and keyed host lifetime. The shared host-aware body selection and a plain +group-child column make all four pass. Default tool fallback and output updates +remain covered; inserting/removing tools preserves child identity and the user's +disclosure choice. Removed host children dispose once while the caller message +stream continues to accept changes. + +The focused 24-case lui_ai cohort and the complete 147-case test_lui native +target pass. Melange compilation through @web passes, as does the OCaml AST +64-line check on the changed source/test modules. No Dune file, warning setting +or unsafe cast changed. Actual consumer browser acceptance follows publication +and installed-package review; it is not established by the native checks alone. + +The new decision passes its individual check. The repository-wide document check +still reports two existing legacy paths, 001-implementation-review_report.md +and 002-review-fixes.md; this change does not rename unrelated historical files +or claim that repository-wide check passed. diff --git a/test/test_lui_ai.ml b/test/test_lui_ai.ml index d9dc5076..51dba2fb 100644 --- a/test/test_lui_ai.ml +++ b/test/test_lui_ai.ml @@ -186,6 +186,111 @@ let test_tool_group_rendering () = (List.exists (( = ) "Worked through 2 steps") (text_nodes s)); drive_dispose s +let host_attrs (item : M.item) = + [ "data-event-id", item.id; "data-current-text", item.text ] + +let host_tool ?(on_press = fun _ -> ()) item_s = + if (Lui_elements.sample item_s).M.id <> "host" then None + else + Some + (Lui_elements.button ~text:"Host tool details" + ~accessibility_identifier:"host-tool-details" + ~on_press:(fun _ -> on_press (Lui_elements.get item_s)) []) + +let host_transcript ?(group_tools = true) ?on_press () _ctx source _send = + Lui_ai.Transcript.transcript ~messages:source ~group_tools + ~render_item:(host_tool ?on_press) ~item_data_attrs:host_attrs + ~item_class:(fun _ -> "host-item") () + +let test_grouped_host_renderer () = + let initial = [ tool_item "host" `in_progress; tool_item "fallback" `pending ] in + let s = drive_mount ~initial ~view:(host_transcript ()) in + Alcotest.(check bool) "host renderer is used inside the group" true + (List.mem "Host tool details" (text_nodes s)); + Alcotest.(check bool) "default tool renderer remains available" true + (List.exists (fun text -> has_sub text "Run fallback") (text_nodes s)); + let host = find_by_prop s "accessibility-identifier" + (StringValue "lui-ai-item:host") in + Alcotest.(check string) "group member is a noninteractive container" "column" + host.Drive.Model.kind; + Alcotest.(check (option string)) "host row class is preserved" (Some "host-item") + (match node_prop host "style-class" with Some (StringValue x) -> Some x | _ -> None); + Alcotest.(check bool) "host row metadata is preserved" true + (match node_prop host "data-attrs" with + | Some (StringValue x) -> has_sub x "data-event-id" && has_sub x "host" + | _ -> false); + drive_dispose s + +let test_grouped_host_updates () = + let clicked = ref "" in + let initial = [ tool_item "host" `in_progress; tool_item "fallback" `in_progress ] in + let view = host_transcript ~on_press:(fun item -> clicked := item.M.text) () in + let s = drive_mount ~initial ~view in + let host () = find_by_prop s "accessibility-identifier" + (StringValue "lui-ai-item:host") in + let before = (host ()).Drive.Model.id in + drive_send_model s (fun items -> M.append_text items "host" "fresh host content"); + drive_send_model s (fun items -> + M.update_tool items (update "fallback" ~output:"fresh default output" ())); + Alcotest.(check int) "streaming retains grouped row identity" before (host ()).id; + Alcotest.(check bool) "grouped metadata follows the current item" true + (match node_prop (host ()) "data-attrs" with + | Some (StringValue x) -> has_sub x "fresh host content" | _ -> false); + Drive.Session.press s + (find_by_prop s "accessibility-identifier" (StringValue "host-tool-details")).id; + Alcotest.(check string) "host callback reads current borrowed item" + "fresh host content" !clicked; + Alcotest.(check bool) "default grouped tool content still updates" true + (List.mem "fresh default output" (text_nodes s)); + drive_dispose s + +let test_single_host_metadata_updates () = + let initial = [ tool_item "host" `in_progress ] in + let s = drive_mount ~initial ~view:(host_transcript ~group_tools:false ()) in + let host () = find_by_prop s "accessibility-identifier" + (StringValue "lui-ai-item:host") in + let before = (host ()).Drive.Model.id in + drive_send_model s (fun items -> M.append_text items "host" "new single metadata"); + Alcotest.(check int) "single row keeps identity" before (host ()).id; + Alcotest.(check bool) "single metadata follows the same item contract" true + (match node_prop (host ()) "data-attrs" with + | Some (StringValue x) -> has_sub x "new single metadata" | _ -> false); + drive_dispose s + +let test_grouped_host_lifetime () = + let mounted = ref 0 and disposed = ref [] in + let renderer source = + let id = (Lui_elements.sample source).M.id in + Some (Lui_elements.scoped (fun context -> + incr mounted; + Signal.on_dispose context.Lui_ui.ui_scope (fun () -> disposed := id :: !disposed); + Lui_elements.button ~text:id ~accessibility_identifier:("host:" ^ id) [])) in + let view _ctx source _send = + Lui_ai.Transcript.transcript ~messages:source ~render_item:renderer () in + let t1 = tool_item "t1" `in_progress and t2 = tool_item "t2" `pending in + let s = drive_mount ~initial:[t1;t2] ~view in + let group = find_by_prop s "accessibility-identifier" + (StringValue "lui-ai-tool-group") in + let t2_node = find_by_prop s "accessibility-identifier" (StringValue "host:t2") in + Drive.Session.toggle s group.id false; + drive_send_model s (fun _ -> [t1;tool_item "t3" `in_progress;t2]); + Alcotest.(check int) "group insertion retains custom tool" t2_node.id + (find_by_prop s "accessibility-identifier" (StringValue "host:t2")).id; + Alcotest.(check bool) "user disclosure survives active updates" true + (node_prop group "selected" = Some (BoolValue false)); + Alcotest.(check int) "only the added custom child mounts" 3 !mounted; + drive_send_model s (fun items -> List.filter (fun item -> item.M.id <> "t2") items); + Alcotest.(check (list string)) "removed child is disposed once" ["t2"] !disposed; + drive_send_model s (fun _ -> []); + Alcotest.(check (list string)) "all group children are disposed" ["t1";"t2";"t3"] + (List.sort String.compare !disposed); + drive_send_model s (fun _ -> [assistant "after" "caller stream remains alive"]); + Alcotest.(check bool) "group disposal does not dispose caller source" true + (List.mem "after" (text_nodes s)); + drive_dispose s; + Alcotest.(check int) "each mounted host view is disposed exactly once" 4 + (List.length !disposed) + (* {1 Anchored follow (virtual_list by default)} *) let test_autoscroll_follow () = @@ -555,6 +660,10 @@ let tests = Alcotest.test_case "reorder/dup identity" `Quick test_keyed_reorder_dup; Alcotest.test_case "tool group" `Quick test_tool_group_rendering; + Alcotest.test_case "grouped host renderer" `Quick test_grouped_host_renderer; + Alcotest.test_case "grouped host updates" `Quick test_grouped_host_updates; + Alcotest.test_case "single host metadata updates" `Quick test_single_host_metadata_updates; + Alcotest.test_case "grouped host lifetime" `Quick test_grouped_host_lifetime; Alcotest.test_case "autoscroll follow" `Quick test_autoscroll_follow; Alcotest.test_case "reach-top hook" `Quick test_reach_top_hook; Alcotest.test_case "host-driven viewport" `Quick