justinpark commented on code in PR #43093:
URL: https://github.com/apache/superset/pull/43093#discussion_r3834733860
##########
superset-frontend/src/core/chat/ChatProvider.ts:
##########
@@ -29,6 +30,62 @@ import { createValueEventEmitter, createEventEmitter } from
'../utils';
type Chat = chatApi.Chat;
type DisplayMode = chatApi.DisplayMode;
+type ClientTool = chatApi.ClientTool;
+type ClaudeToolSpec = chatApi.ClaudeToolSpec;
+type ClientToolsFormat = chatApi.ClientToolsFormat;
+
+// The real value backing @apache-superset/core's `declare const
+// ClientToolsFormat` — that package only ever declares (see its own docs on
+// why this isn't a TS `enum`); this is the actual object attached to
+// `window.superset.chat.ClientToolsFormat` (re-exported from ./index).
+export const ClientToolsFormat = {
+ Claude: 'claude',
+ AgUi: 'ag-ui',
+ CopilotKit: 'copilot-kit',
+ Codex: 'codex',
+} as const;
+
+// AgUi/CopilotKit/Codex have no real transform below — see
+// @apache-superset/core's ClientToolsFormat docs for why (no framework in
+// this codebase actually talks to any of them yet, so there's no verified
+// target shape to convert to). Throwing a clear, named error beats either
+// silently returning the native ClientTool[] (wrong shape, and callers can
+// already get that from a plain getTools()) or returning an empty array
+// (looks like "this source has no tools" instead of "this format isn't
+// implemented").
+function notYetImplemented(
+ formatKey: keyof typeof ClientToolsFormat,
+): () => never {
+ return () => {
+ throw new Error(
+ `[Superset] chat.getTools(chat.ClientToolsFormat.${formatKey}) is ` +
+ 'not yet implemented — no framework in this codebase talks to ' +
+ 'this format yet, so there is no verified target shape to convert ' +
+ 'to. Add a real transform to CLIENT_TOOLS_FORMATTERS in ' +
+ 'ChatProvider.ts once there is one to verify against, rather than ' +
+ 'guessing at it here.',
+ );
+ };
+}
+
+// One entry per ClientToolsFormat member — see that constant's own docs for
+// why only Claude has a real transform. Keeping each target's transform (or
+// placeholder) here, keyed by the same object, is what makes adding a real
+// one later a single changed entry rather than a change to getTools()
+// itself.
+const CLIENT_TOOLS_FORMATTERS: {
+ [K in ClientToolsFormat]: (tools: ClientTool[]) => unknown[];
+} = {
+ [ClientToolsFormat.Claude]: (tools: ClientTool[]): ClaudeToolSpec[] =>
+ tools.map(tool => ({
+ name: tool.name,
+ description: tool.description,
+ input_schema: tool.inputSchema,
+ })),
+ [ClientToolsFormat.AgUi]: notYetImplemented('AgUi'),
+ [ClientToolsFormat.CopilotKit]: notYetImplemented('CopilotKit'),
+ [ClientToolsFormat.Codex]: notYetImplemented('Codex'),
+};
Review Comment:
Good point. I agree — for now, let's keep the default format only and drop
this formatter. The support for alternative formats is still evolving as each
one finds its own strengths, so it makes more sense to revisit this once that
landscape stabilizes rather than generalizing prematurely. I'll update
accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
