feat(frontend): render canonical gateway tools identically to UI-added ones#5140
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds design documentation for fixing canonical gateway tool rendering in the playground, and implements a shared ChangesGateway Tool Rendering Design Docs
Gateway Tool Parsing & Rendering Implementation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant List as ToolManagementList
participant Utils as toolUtils.parseGatewayTool
participant FormView as ToolFormView
participant CanonicalForm as CanonicalGatewayToolForm
participant API as useToolActionDetail
List->>Utils: parseGatewayTool(item)
Utils-->>List: {encoding, provider, integration, action}
List->>FormView: render row / open drill-in
FormView->>Utils: check gateway.encoding === "canonical"
alt canonical
FormView->>CanonicalForm: render
CanonicalForm->>API: fetch action detail
API-->>CanonicalForm: catalog schema or error
alt resolved
CanonicalForm-->>FormView: legacy-shaped tool via FunctionToolForm
else unresolved
CanonicalForm-->>FormView: warning + raw JSON fail-safe
end
else legacy
FormView-->>FormView: render FunctionToolForm directly
end
sequenceDiagram
participant Drawer as AgentIntegrationDrawer
participant Hook as useAgentTools
participant Utils as toolUtils.gatewayToolIdentity
Hook->>Utils: derive selectedGatewayIds from tools
Utils-->>Hook: identity set
Hook-->>Drawer: selectedGatewayIds, removeGatewayToolByIdentity
Drawer->>Utils: gatewayToolIdentity(catalogAction)
Utils-->>Drawer: identity
alt identity already selected
Drawer->>Hook: removeGatewayToolByIdentity(identity)
Hook-->>Drawer: removes first matching tool
else not selected
Drawer->>Drawer: onAddTool(...)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| - **Option A — humanize-only (minimal).** Show the humanized action, the identity, and | ||
| the local schema if the tool happens to carry one (legacy does; canonical shows "schema | ||
| resolved at run time"). No network call. Simplest, ships the rendering fix cleanly. | ||
| - **Option B — fetch catalog detail on drill-in.** On open, call the existing |
There was a problem hiding this comment.
Let's use option B and make sure that the UI for tools in the playground is shown exactly the same for both the tools created from the UI and the ones created through the agent. This is through the canonical view that is outside in the UI, but also on the drilling view.
There was a problem hiding this comment.
Decided: Option B (fetch catalog detail). Folded into plan.md Question 2 and the new "Decisions" section: on drill-in we call the existing useToolActionDetail(integration, action) hook and populate the view with the real description + read-only input-schema preview, so a canonical tool shows EXACTLY the same as a legacy one in both the outside list and the drill-in. Open question #1 closed in status.md and context.md's review round. (commit f1dc65d)
|
|
||
| ## Question 3 — Add / remove / dedupe | ||
|
|
||
| The drawer must treat canonical and legacy entries as the same tool for the same |
There was a problem hiding this comment.
That is actually not the responsibility of the front-end. The front-end should show what is, even if the same thing is duplicated. It's not the responsibility of the front-end.
The only thing that is the responsibility of the front-end is that you cannot add again a tool that already exists, so it's not about deduplicating. When I'm adding something, I make sure that I show it as added. When you see the list of tools, I show that things are already added, so that I cannot hide it twice. Now, if an agent or something programmatically adds it twice, that's not the responsibility of the front-end.
If there are two of them and then you remove again, the responsibility of the front-end is just removing the things that you selected for removing. If there are two, also, it's not the responsibility of the front-end to deduplicate them. It should show both, so I think the only path is the addition path that is kind of important for the front-end to care about.
There was a problem hiding this comment.
Agreed and applied. The FE does not deduplicate — showing what exists, duplicates included, is correct. Question 3 in plan.md is rewritten to the narrow "add path" scope: identity (gatewayToolIdentity) serves only the drawer's added-state, double-add prevention, and toggle-off of the matched entry. Removal deletes exactly the selected entry. Dropped all display-dedupe and removal-dedupe language (helper comments and interface-role notes too). (commit f1dc65d)
| Note: what the drawer newly **adds** stays in today's legacy shape unless we adopt | ||
| Question 4. Reading canonical is independent of writing it. | ||
|
|
||
| ## Question 4 — Convergence (OPEN — do not decide unilaterally) |
There was a problem hiding this comment.
I would not change the logic for now.
There was a problem hiding this comment.
Decided: deferred — "I would not change the logic for now." Question 4 in plan.md now records the drawer keeps writing the legacy function.name shape on add; no write-path change in this work. Read-side canonical support is unaffected (both shapes render/drill-in regardless of what the drawer writes). Open question #2 closed in status.md and context.md's review round. (commit f1dc65d)
| schema editor is misleading for **both** encodings (the legacy Form already papers over | ||
| this with a "couldn't load the schema" warning banner, `ToolFormView.tsx:149`). | ||
|
|
||
| Proposed: route gateway tools (both shapes, detected by `parseGatewayTool`) to a small |
There was a problem hiding this comment.
We have already kind of a view of opening tools in the drawer. We should not change it. It should stay the same as far as that tool has been fetched and exists, and we have all the data to show it.
If the tool has not been fetched correctly, like there is an error, then we might show it as a JSON with some sort of warning, especially for a gateway tool. We are not able to fetch it, or whatever, so we just show the raw thing as JSON, as we do right now.
That's actually an important thing that we need to take care of: the case where an agent or somebody programmatically writes a tool that does not exist or something that we cannot fetch the catalog details on. We are uncertain that it exists for gateway tools, obviously here.
There was a problem hiding this comment.
Applied. The new GatewayToolDetailView is rejected — we reuse the existing view. Question 2 in plan.md now routes a resolvable canonical entry to the SAME existing gateway drill-in a legacy entry gets (populated via the Option-B catalog fetch), no new component. Added your new requirement as an explicit design item with its own plan slice + test: when the catalog fetch fails or the action cannot be confirmed (an agent wrote a nonexistent tool), fail safe to the raw JSON view as today plus a warning that the tool could not be resolved. That fail-safe is the only place a canonical tool may look different from a legacy one. (commit f1dc65d)
| its object-level superset, and `parseGatewayFunctionName` stays as the string-level | ||
| primitive it composes. | ||
|
|
||
| ## Question 1 — Rendering (row descriptor + grouping) |
There was a problem hiding this comment.
So the rendering and the grouping, I'm not sure I understand what's what. What do you mean? It looks like implementation details. From a product perspective, we should not change anything. It should exactly look the same before and after. The only cases where things will be a little bit different are, as you've seen, kind of the second question. We are not able to fetch the details for that tool, and then we would show it a little bit raw, like we show it right now. So kind of same fail-safe. Overall, try to kind of simplify the code and think about how to simplify the logic while obviously keeping the functionality as is.
There was a problem hiding this comment.
Reframed. plan.md now leads with the product invariant: nothing about the tool UI changes for the user — same rows, same grouping, same drill-in — and an agent-created tool looks exactly like a UI-created one. The shared parser is presented as a simplification (collapsing two detection paths into one), not a product change. Question 1 opens with that invariant. The only visible divergence anywhere is the unresolvable-tool fail-safe (see the reply on Question 2). (commit f1dc65d)
mmabrouk
force-pushed
the
docs/gateway-tool-rendering
branch
from
f1dc65d to
7e230b4
Compare
mmabrouk
force-pushed
the
docs/gateway-tool-rendering
branch
from
7e230b4 to
b1f2e8b
Compare
mmabrouk
changed the title
dosubot
Bot
added
size:XL
| } | ||
|
|
||
| /** Normalize either encoding of a connected-app tool into one view. */ | ||
| export function parseGatewayTool(tool: unknown): ParsedGatewayTool | null { |
There was a problem hiding this comment.
The shared seam. Every consumer (row, grouping, drill-in, drawer) normalizes both encodings through this one parser, so they can't disagree. Canonical reads the discriminated object; legacy composes the shared parseGatewayToolSlug. Returns null for anything that isn't a connected-app tool.
| return {provider, integration, action, connection} | ||
| /** Stable identity for the drawer's added-state, independent of encoding. Excludes | ||
| * permission (policy, not identity) and encoding. */ | ||
| export function gatewayToolIdentity(view: ParsedGatewayTool): string { |
There was a problem hiding this comment.
Stable add-path identity, independent of encoding. Excludes permission (policy, not identity) and encoding, and joins with a NUL because a connection slug can contain a dot. This is what lets the drawer match a canonical entry against a UI action across encodings.
| // Third-party / gateway tool: tools__provider__integration__action__connection. | ||
| const gateway = fnName ? parseGatewayFunctionName(fnName) : null | ||
| // Third-party / gateway tool: canonical object or legacy slug. | ||
| const gateway = parseGatewayTool(t) |
There was a problem hiding this comment.
Only the detection widened: was parseGatewayFunctionName(fnName), now parseGatewayTool(t). The gateway branch body below is unchanged, so a canonical object reaches the exact same row a legacy tool gets. The reference branch still runs first.
| // Function, workflow-reference, and gateway tools (either encoding) have a structured Form; | ||
| // only bare builtin/provider tools (a naked `type`) stay JSON-only. A canonical gateway | ||
| // object opens the Form via `parseGatewayTool`; a legacy one is already a function tool. | ||
| editView: (item) => |
There was a problem hiding this comment.
editView is synchronous, so it only decides that a canonical gateway object opens the Form (legacy already did). It does NOT decide resolvable-vs-failsafe; that needs the async catalog fetch and lives in the mounted form body. jsonOnly stays false so the JSON toggle remains the escape hatch.
| * edits. Fail-safe: an action/connection that can't be resolved falls back to today's raw-JSON view | ||
| * plus a warning (the drawer's JSON toggle stays the editable escape hatch). | ||
| */ | ||
| function CanonicalGatewayToolForm({ |
There was a problem hiding this comment.
Canonical-only wrapper. It resolves the catalog (Option B), then feeds FunctionToolForm the exact legacy function shape a UI-added tool would have, so the drill-in is pixel-identical to a legacy one. Legacy tools never enter here (router checks encoding === canonical). The on-disk draft stays canonical; the synthesized shape is display-only.
| </div> | ||
| ) | ||
| } | ||
| if (!legacyValue) { |
There was a problem hiding this comment.
Fail-safe: a terminal !isLoading && !action shows a warning plus read-only raw JSON. Loading returns a spinner first, so no flash of the wrong view. error is intentionally not consulted, so a stale background refetch error can't flip a resolved tool to the warning.
| ) | ||
|
|
||
| // Remove EXACTLY ONE identity match (toggle-off) — never all duplicates, per the design. | ||
| const removeGatewayToolByIdentity = useCallback( |
There was a problem hiding this comment.
Toggle-off removes EXACTLY ONE identity match (the removed flag), never all duplicates. Derived from the same tools snapshot as selectedGatewayIds, so the two can't drift. handleRemoveToolByName is left for the row remove button's legacy path.
| ) | ||
|
|
||
| // Encoding-independent identity — matches a canonical or legacy entry already in the config. | ||
| const idFor = useCallback( |
There was a problem hiding this comment.
idFor builds the identity from the same fields (provider default composio, integration, action.key, connection slug) that parseGatewayTool re-derives from a stored tool, so a canonical or legacy entry already in the config reads as added. The add write still uses the slug (convergence deferred).
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/packages/agenta-entity-ui/tests/unit/gatewayTool.test.ts (1)
181-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests duplicate hook logic instead of exercising it.
selectedGatewayIdsand the one-match removal filter here are copy-pasted re-implementations of the logic inuseAgentTools.ts(selectedGatewayIds/removeGatewayToolByIdentity), rather than imports of the real functions. If the hook's implementation later diverges (e.g., a bug is introduced in the removal filter), these "regression" tests would keep passing since they only validate the copy, not the production code path.Consider exporting the pure derivation/removal logic (e.g., as standalone functions in
toolUtils.ts) and having bothuseAgentTools.tsand this test import the same implementation.web/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ToolFormView.tsx (1)
187-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
normalizeParameters— consolidate with the add-drawer's implementation.The docstring itself notes this "mirrors the add drawer's normalizeParameters" instead of sharing it. Two independent copies of the same schema-normalization logic will drift silently over time (e.g. a future fix to one won't propagate to the other).
♻️ Suggested consolidation
-function normalizeParameters(inputs: unknown): Record<string, unknown> { - if (!isRecord(inputs)) { - return {type: "object", properties: {}, required: [], additionalProperties: false} - } - const schema = {...(inputs as Record<string, unknown>)} - if (schema.type !== "object") schema.type = "object" - if (!isRecord(schema.properties)) schema.properties = {} - if (!Array.isArray(schema.required)) schema.required = [] - if (typeof schema.additionalProperties !== "boolean") schema.additionalProperties = false - return schema -} +// Moved to `../toolUtils` and imported by both this file and AgentIntegrationDrawer.tsx. +import {normalizeParameters} from "./toolUtils"
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 132d1d5a-0681-4488-b27e-88af61a32a0f
📒 Files selected for processing (14)
docs/design/gateway-tool-rendering/README.mddocs/design/gateway-tool-rendering/context.mddocs/design/gateway-tool-rendering/plan.mddocs/design/gateway-tool-rendering/research.mddocs/design/gateway-tool-rendering/status.mdweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/AgentTemplateControl.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/ToolFormView.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/AgentIntegrationDrawer.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/ToolManagementList.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemDescriptors.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/itemKinds.tsxweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/agentTemplate/useAgentTools.tsweb/packages/agenta-entity-ui/src/DrillInView/SchemaControls/toolUtils.tsweb/packages/agenta-entity-ui/tests/unit/gatewayTool.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/design/gateway-tool-rendering/README.md
- docs/design/gateway-tool-rendering/status.md
New plan-feature workspace for fixing the playground FE read path so it renders connected-app tools written in the canonical type:"gateway" shape, not just the legacy function-name slug the UI writes itself. Frontend read-path only; both encodings are already equivalent server-side (SDK compat + catalog re-enrichment). Design only. Two open questions parked for Mahmoud (drill-in richness; whether the drawer should start writing the canonical shape on add). Claude-Session: https://claude.ai/code/session_01EcGku1uKvh1Yo48ZU2xN5e
…ng, Option B drill-in via the existing view, add-path-only identity, convergence deferred Claude-Session: https://claude.ai/code/session_01EcGku1uKvh1Yo48ZU2xN5e
…d ones
Builder agents persist connected-app tools in the canonical {type:"gateway"}
shape; the playground only understood its own legacy function.name slug, so
canonical entries rendered as "gateway · built-in" JSON blobs. Normalize both
encodings through one shared parser (parseGatewayTool + gatewayToolIdentity) so
the row, grouping, drill-in, and add-drawer all agree. Legacy rendering is byte
-for-byte unchanged; canonical tools now render pixel-identical to UI-added ones,
with a catalog-resolved drill-in (Option B) and a fail-safe for unresolvable
tools. FE read-path only; no backend or SDK changes.
Claude-Session: https://claude.ai/code/session_01EcGku1uKvh1Yo48ZU2xN5e
mmabrouk
force-pushed
the
docs/gateway-tool-rendering
branch
from
b1f2e8b to
9fdb1ab
Compare
1 participant
Context
A builder agent authored three Slack tools in the canonical persisted shape
(
{"type":"gateway","provider":"composio","integration":"slack","action":"OPEN_DM","connection":"slack-pnt",...}).In the playground Tools section all three rendered as "gateway · built-in" rows under a
BUILT-IN header, each opening a raw JSON editor. The expected rendering is a Slack card
under Connected apps with humanized action names ("Open dm") and a proper drill-in.
The cause is a frontend read-path gap. Every consumer keyed connected-app tools off the
legacy encoding the UI writes for itself: an OpenAI function tool whose
function.nameis the slug
tools__provider__integration__action__connection. The canonicaltype:"gateway"object has nofunction, so it fell through to the built-in fallback. Bothencodings are already equivalent server-side (the SDK compat layer and the resolver enrich
description and schema from the live catalog at run time), so this is a read-path fix only.
Before / after (the reported repro)
ToolFormViewa UI-added tool opens, params/description from the catalogWhat changed
One shared parser normalizes both encodings; every consumer reads through it.
toolUtils.ts—parseGatewayTool(tool)(object-level, handles canonical and legacy)and
gatewayToolIdentity(view). The oldparseGatewayFunctionNamenow aliases the sharedparseGatewayToolSlug(it was a duplicate).itemDescriptors.tsx,ToolManagementList.tsx) —describeTooland thelist grouping read through
parseGatewayTool, so canonical tools reach the exact same rowand grouping a legacy tool gets.
itemKinds.tsx,ToolFormView.tsx) — the existingToolFormViewbody wasextracted verbatim into an inner
FunctionToolForm(legacy tools are untouched). Acanonical object routes to a
CanonicalGatewayToolFormwrapper that fetches the catalogdetail (
useToolActionDetail, Option B), synthesizes the exact legacy function shape aUI-added tool would have, and renders
FunctionToolFormwith it. The on-disk draft stayscanonical; the synthesized shape is display-only and nothing persists unless the user edits.
shows a warning plus a read-only raw-JSON view. The drawer's JSON toggle stays the editable
escape hatch.
useAgentTools.ts,AgentIntegrationDrawer.tsx) —selectedGatewayIdsandremoveGatewayToolByIdentity(both derived from the sametoolssnapshot) make the adddrawer match added-state by identity across encodings, count canonical tools, and toggle
off exactly one match.
Decisions folded in (Mahmoud's review + constraint rounds, plus a codex xhigh review)
sources. Legacy rendering is byte-for-byte unchanged; only canonical tools change, from a
JSON blob to the legacy look.
prevention, toggle-off of the matched entry). No display or removal dedup.
the synchronous
itemKinds.editView(which cannot know async catalog state).Scope and risk
already equivalent server-side.
FunctionToolFormunchanged;the add-drawer identity match is behaviourally identical to the old slug match for legacy.
sibling files (
connectionUtils.ts,ProviderCredentialsSection.tsx,useModelHarness.tsx,CustomProviderForm.tsx,ModelNameInput.tsx,DrillInUIContext.tsx) was not touched.ToolSelectorPopover,PromptSchemaControl,ToolItemControl) still keys off the legacy slug;commitDiffidentity keys tools by
function.name, so a canonical tool gets positional identity in thechange-diff layer;
useToolActionDetailis composio-only.How to QA
Live repro on the dev stack: app
019f3d51-1f93-7452-8133-dff2f0d91385, revision "Supporttriage" (
019f3d56-90f3-7870-b1c4-bd67f4313e18) holds three canonical Slack tools.("Open dm", "Send message", "Retrieve message permalink URL"), not "gateway · built-in".
ToolFormViewa UI-added tool opens, withPARAMETERS, Name (slug), and Description from the catalog, and Permission = Allow. Switch
to JSON: the object is still the untouched canonical
{type:"gateway",...}(read-pathonly).
JSON, no crash.
"3 app tools added". Toggle one off (count 2), toggle it back on (count 3).
Package checks (from
web/):All green (158 unit tests). Steps 1, 2, 4, 5 were verified in a real browser on the live
repro; step 3 is verified by review (the in-app JSON editor blocks synthetic edits and the
package has no render-test harness) and is a terminal
!isLoading && !actionbranch.https://claude.ai/code/session_01EcGku1uKvh1Yo48ZU2xN5e