codeant-ai-for-open-source[bot] commented on code in PR #45392:
URL: https://github.com/apache/superset/pull/45392#discussion_r4298638941
##########
superset-frontend/src/core/chat/ChatProvider.ts:
##########
@@ -190,10 +221,52 @@ class ChatProvider {
return this.resizePanelEmitter.subscribe;
}
+ /**
+ * Registers a single client-side tool — mirrors commands.ts's
+ * registerCommand exactly: keyed by the tool's own `name` (fully-qualified,
+ * author-chosen — nothing prefixes or validates it here, same as a
+ * `Command.id`), warns and overwrites on a duplicate name, and the
+ * returned Disposable removes it by name unconditionally on dispose.
+ *
+ * Registering after registerChat() is safe — nothing here depends on call
+ * order. (An earlier version of this method warned about that ordering,
+ * back when ChatPanel snapshotted `chat.getTools()` once at mount via
+ * `useMemo(..., [])`; the actual fix was moving that snapshot to send-time
+ * in ChatPanel itself, which made the order genuinely not matter rather
+ * than just warning about it.)
+ */
+ public registerClientTool(tool: ClientTool): Disposable {
+ const { name } = tool;
+ if (this.clientTools.has(name)) {
+ logging.warn(
+ `[Superset] Client tool "${name}" is already registered. ` +
+ 'Overwriting the existing tool.',
+ );
+ }
+ this.clientTools.set(name, tool);
+ return new Disposable(() => {
+ this.clientTools.delete(name);
Review Comment:
Yes—`this.clientTools.get(name) === tool` prevents an older registration
from deleting a newer one when they use distinct tool objects. For complete
safety, use a unique registration record instead: the same tool object could be
registered twice, and an identity check on the tool itself would not
distinguish those registrations.
For example, store `{ tool }` as the map value and delete only if the
current value is the same record returned by that registration. Then an older
Disposable cannot remove a newer registration, even when both registrations use
the same object.
##########
superset-frontend/src/core/clientTools/sqlLab/index.ts:
##########
@@ -0,0 +1,238 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import type { chat, sqlLab as sqlLabApi } from '@apache-superset/core';
+import { navigation } from 'src/core/navigation';
+import { sqlLab } from 'src/core/sqlLab';
+
+const emptySchema = {
+ type: 'object',
+ properties: {},
+ additionalProperties: false,
+};
+const editorIdSchema = {
+ type: 'object',
+ properties: { editor_id: { type: 'string', minLength: 1 } },
+ required: ['editor_id'],
+ additionalProperties: false,
+};
+
+/** Validates inputs even when a consumer invokes a handler without schema
validation. */
+function inputObject(input: unknown): Record<string, unknown> {
+ if (!input || typeof input !== 'object' || Array.isArray(input)) {
+ throw new Error('Expected an object input');
+ }
+ return input as Record<string, unknown>;
+}
+
+/** Reads a required string, allowing an empty SQL buffer. */
+function stringInput(input: Record<string, unknown>, key: string): string {
+ const value = input[key];
+ if (typeof value !== 'string' || (key !== 'sql' && !value.trim())) {
+ throw new Error(`Expected a string for "${key}"`);
+ }
+ return value;
+}
+
+/** Resolves the active tab at invocation time. */
+function currentTab(): sqlLabApi.Tab {
+ const tab = sqlLab.getCurrentTab();
+ if (!tab) throw new Error('No active SQL editor tab');
+ return tab;
+}
+
+/** Resolves a target before APIs that silently ignore missing tab IDs are
called. */
+function targetTab(input: unknown): sqlLabApi.Tab {
+ const editorId = stringInput(inputObject(input), 'editor_id');
+ const tab = sqlLab.getTabs().find(item => item.id === editorId);
+ if (!tab) throw new Error(`SQL editor "${editorId}" not found`);
+ return tab;
+}
+
+/** Serializes tab metadata without waiting for inactive editors to mount. */
+function tabMetadata(tab: sqlLabApi.Tab) {
+ return {
+ editor_id: tab.id,
+ title: tab.title,
+ databaseId: tab.databaseId,
+ catalog: tab.catalog,
+ schema: tab.schema,
+ };
+}
+
+/** SQL Lab tools backed exclusively by the existing frontend API. */
+const sqlLabTools: chat.ClientTool[] = [
+ {
+ name: 'sqlLab__get_current_editor_id',
+ description: 'Get the ID of the active SQL Lab editor tab.',
+ inputSchema: emptySchema,
+ annotations: { readOnlyHint: true },
+ handler: () => ({ success: true, editor_id: currentTab().id }),
+ },
+ {
+ name: 'sqlLab__get_editor_tabs',
+ description:
+ 'List open SQL Lab tabs and their database, catalog, and schema. Switch
tabs before reading or editing their SQL.',
+ inputSchema: emptySchema,
+ annotations: { readOnlyHint: true },
+ handler: () => ({
+ success: true,
+ editors: sqlLab.getTabs().map(tabMetadata),
+ }),
+ },
+ {
+ name: 'sqlLab__get_databases',
+ description: 'List database IDs and names already loaded in SQL Lab.',
+ inputSchema: emptySchema,
+ annotations: { readOnlyHint: true },
+ handler: () => ({
+ success: true,
+ databases: sqlLab.getDatabases().map(({ id, name }) => ({ id, name })),
+ }),
+ },
+ {
+ name: 'sqlLab__get_editor_sql',
+ description: 'Read the active SQL Lab editor SQL, selection, and
metadata.',
+ inputSchema: emptySchema,
+ annotations: { readOnlyHint: true },
+ handler: async () => {
+ const tab = currentTab();
+ const editor = await tab.getEditor();
Review Comment:
Yes—the handlers should not wait indefinitely. A **tab-close signal is
preferable** to a timeout: race `getEditor()` against the captured tab’s close
event, clean up the listener whichever settles first, and return a structured
failure if the tab closes. Apply this to both `get_editor_sql` and
`update_editor_sql`.
A timeout is a reasonable fallback only if SQL Lab has no close event, but
it introduces an arbitrary delay and may fail for a slow-to-mount editor. If
the API exposes no lifecycle signal, adding a cancellable `getEditor()` or
tab-close event would be the more reliable fix. This change is warranted
because otherwise a closed tab can leave the tool call pending forever.
--
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]
