This is an automated email from the ASF dual-hosted git repository. EnxDev pushed a commit to branch test/chatbot-local in repository https://gitbox.apache.org/repos/asf/superset.git
commit 1645a9652f6dea9246605c88c83b1c96579fc123 Author: Enzo Martellucci <[email protected]> AuthorDate: Wed May 27 23:36:05 2026 +0200 fix(extensions): address PR review — rollback, import cleanup, tests, guard - ExtensionsList: snapshot previous settings before optimistic write; rollback setSettings + notifyExtensionSettingsChanged in .catch() so a failed PUT leaves the UI consistent with server state. Drop setSettings(json.result) from .then() — optimistic write is source of truth. Switch onClick → onChange. Consolidate Switch/Select/etc into single @superset-ui/core/components import. - ChatbotMount: revert undefined loading gate (immediate render, fall back on fetch error); guard json.result with ?? fallback; merge React imports; promote ChatbotRenderer comment to JSDoc. - Tests: add getActiveChatbot coverage for admin-pin, stale-pin fallback, enabled-filter exclusion, all-disabled. Add ChatbotMount test for provider function throwing synchronously. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --- .../components/ChatbotMount/ChatbotMount.test.tsx | 16 +++++ .../src/components/ChatbotMount/index.tsx | 35 +++++++---- superset-frontend/src/core/chatbot/index.test.ts | 71 ++++++++++++++++++++++ .../src/extensions/ExtensionsList.tsx | 24 +++++--- 4 files changed, 125 insertions(+), 21 deletions(-) diff --git a/superset-frontend/src/components/ChatbotMount/ChatbotMount.test.tsx b/superset-frontend/src/components/ChatbotMount/ChatbotMount.test.tsx index ba546005398..a039c30a157 100644 --- a/superset-frontend/src/components/ChatbotMount/ChatbotMount.test.tsx +++ b/superset-frontend/src/components/ChatbotMount/ChatbotMount.test.tsx @@ -89,3 +89,19 @@ test('isolates a failing chatbot so it does not crash the host', () => { // The host-owned error boundary catches the failure; render does not throw. expect(() => render(<ChatbotMount />)).not.toThrow(); }); + +test('isolates a chatbot whose provider function itself throws', () => { + disposables.push( + views.registerView( + { id: 'superset.chatbot', name: 'Superset Chatbot' }, + CHATBOT_LOCATION, + () => { + throw new Error('provider blew up'); + }, + ), + ); + + // ChatbotRenderer wraps provider() in a component so ErrorBoundary catches + // synchronous throws from the provider function, not just from its output. + expect(() => render(<ChatbotMount />)).not.toThrow(); +}); diff --git a/superset-frontend/src/components/ChatbotMount/index.tsx b/superset-frontend/src/components/ChatbotMount/index.tsx index 3eaec221741..c0ff50ad9ec 100644 --- a/superset-frontend/src/components/ChatbotMount/index.tsx +++ b/superset-frontend/src/components/ChatbotMount/index.tsx @@ -16,8 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect, useMemo, useState, useSyncExternalStore } from 'react'; -import { ReactElement } from 'react'; +import { + type ReactElement, + useCallback, + useEffect, + useMemo, + useState, + useSyncExternalStore, +} from 'react'; import { SupersetClient } from '@superset-ui/core'; import { css, useTheme } from '@apache-superset/core/theme'; import { ErrorBoundary } from 'src/components/ErrorBoundary'; @@ -27,20 +33,25 @@ import { subscribeToExtensionSettings } from 'src/core/extensions'; const CHATBOT_EDGE_MARGIN = 24; -type ActiveChatbot = { provider: () => ReactElement }; - -// Renders the provider as a component so ErrorBoundary catches provider-level throws. +/** + * Wraps the chatbot provider in a React component so that ErrorBoundary can + * catch synchronous throws from the provider function itself. Calling + * `provider()` inline (e.g. `{activeChatbot.provider()}`) would throw outside + * React's render boundary and crash the host. + */ const ChatbotRenderer = ({ provider }: { provider: () => ReactElement }) => provider(); const ChatbotMount = () => { const theme = useTheme(); - // null = settings not yet loaded; don't render anything until settings arrive. - const [adminSelectedId, setAdminSelectedId] = useState<string | null | undefined>(undefined); + const [adminSelectedId, setAdminSelectedId] = useState<string | null>(null); const [enabledMap, setEnabledMap] = useState<Record<string, boolean>>({}); const applySettings = useCallback( - (settings: { active_chatbot_id: string | null; enabled: Record<string, boolean> }) => { + (settings: { + active_chatbot_id: string | null; + enabled: Record<string, boolean>; + }) => { setAdminSelectedId(settings.active_chatbot_id ?? null); setEnabledMap(settings.enabled ?? {}); }, @@ -52,10 +63,11 @@ const ChatbotMount = () => { SupersetClient.get({ endpoint: '/api/v1/extensions/settings' }) .then(({ json }) => { if (cancelled) return; - applySettings(json.result); + applySettings(json.result ?? { active_chatbot_id: null, enabled: {} }); }) .catch(() => { // Settings fetch failure is non-fatal — fall back to first-to-register. + // enabledMap stays {} which getActiveChatbot treats as all-enabled. setAdminSelectedId(null); }); return () => { @@ -71,10 +83,7 @@ const ChatbotMount = () => { ); const activeChatbot = useMemo( - () => - adminSelectedId === undefined - ? null - : getActiveChatbot(adminSelectedId, enabledMap), + () => getActiveChatbot(adminSelectedId, enabledMap), [adminSelectedId, enabledMap, registryVersion], ); diff --git a/superset-frontend/src/core/chatbot/index.test.ts b/superset-frontend/src/core/chatbot/index.test.ts index 0b8d6f2f6ec..80cd1d51e95 100644 --- a/superset-frontend/src/core/chatbot/index.test.ts +++ b/superset-frontend/src/core/chatbot/index.test.ts @@ -94,3 +94,74 @@ test('getActiveChatbot stops resolving a chatbot once it is disposed', () => { expect(getActiveChatbot()).toBeUndefined(); }); + +test('getActiveChatbot honours the admin-pinned selection', () => { + const firstProvider = () => React.createElement('div', null, 'First'); + const secondProvider = () => React.createElement('div', null, 'Second'); + disposables.push( + views.registerView( + { id: 'first.chatbot', name: 'First Chatbot' }, + CHATBOT_LOCATION, + firstProvider, + ), + views.registerView( + { id: 'second.chatbot', name: 'Second Chatbot' }, + CHATBOT_LOCATION, + secondProvider, + ), + ); + + const active = getActiveChatbot('second.chatbot'); + expect(active?.id).toBe('second.chatbot'); + expect(active?.provider).toBe(secondProvider); +}); + +test('getActiveChatbot falls back to first-registered when pinned id is unknown', () => { + const provider = () => React.createElement('div', null, 'First'); + disposables.push( + views.registerView( + { id: 'first.chatbot', name: 'First Chatbot' }, + CHATBOT_LOCATION, + provider, + ), + ); + + // 'stale.chatbot' was once the admin pin but is no longer registered. + const active = getActiveChatbot('stale.chatbot'); + expect(active?.id).toBe('first.chatbot'); +}); + +test('getActiveChatbot excludes disabled extensions before applying admin pin', () => { + const firstProvider = () => React.createElement('div', null, 'First'); + const secondProvider = () => React.createElement('div', null, 'Second'); + disposables.push( + views.registerView( + { id: 'first.chatbot', name: 'First Chatbot' }, + CHATBOT_LOCATION, + firstProvider, + ), + views.registerView( + { id: 'second.chatbot', name: 'Second Chatbot' }, + CHATBOT_LOCATION, + secondProvider, + ), + ); + + // Admin pinned second, but second is disabled — should fall back to first. + const active = getActiveChatbot('second.chatbot', { + 'second.chatbot': false, + }); + expect(active?.id).toBe('first.chatbot'); +}); + +test('getActiveChatbot returns undefined when all candidates are disabled', () => { + disposables.push( + views.registerView( + { id: 'superset.chatbot', name: 'Superset Chatbot' }, + CHATBOT_LOCATION, + () => React.createElement('div', null, 'Chatbot'), + ), + ); + + expect(getActiveChatbot(null, { 'superset.chatbot': false })).toBeUndefined(); +}); diff --git a/superset-frontend/src/extensions/ExtensionsList.tsx b/superset-frontend/src/extensions/ExtensionsList.tsx index 9660ab5b867..1b8083c2e55 100644 --- a/superset-frontend/src/extensions/ExtensionsList.tsx +++ b/superset-frontend/src/extensions/ExtensionsList.tsx @@ -27,8 +27,11 @@ import { useSyncExternalStore, } from 'react'; import { SupersetClient } from '@superset-ui/core'; -import { ConfirmStatusChange, Tooltip } from '@superset-ui/core/components'; -import { Switch } from '@superset-ui/core/components/Switch'; +import { + ConfirmStatusChange, + Switch, + Tooltip, +} from '@superset-ui/core/components'; import { Icons } from '@superset-ui/core/components/Icons'; import { useListViewResource } from 'src/views/CRUD/hooks'; import { createErrorHandler } from 'src/views/CRUD/utils'; @@ -102,18 +105,23 @@ const ExtensionsList: FunctionComponent<ExtensionsListProps> = ({ const saveSettings = useCallback( (patch: Partial<ExtensionSettings>) => { - const next = { ...settingsRef.current, ...patch }; + const previous = settingsRef.current; + const next = { ...previous, ...patch }; setSettings(next); + notifyExtensionSettingsChanged(next); SupersetClient.put({ endpoint: '/api/v1/extensions/settings', jsonPayload: next, }) - .then(({ json }) => { - setSettings(json.result); - notifyExtensionSettingsChanged(json.result); + .then(() => { addSuccessToast(t('Settings saved.')); }) - .catch(() => addDangerToast(t('Failed to save extension settings.'))); + .catch(() => { + // Rollback optimistic update so UI stays consistent with server state. + setSettings(previous); + notifyExtensionSettingsChanged(previous); + addDangerToast(t('Failed to save extension settings.')); + }); }, [addDangerToast, addSuccessToast], ); @@ -232,7 +240,7 @@ const ExtensionsList: FunctionComponent<ExtensionsListProps> = ({ <Switch data-test="toggle-enabled" checked={settings.enabled[id] ?? true} - onClick={(checked: boolean) => toggleEnabled(id, checked)} + onChange={(checked: boolean) => toggleEnabled(id, checked)} size="small" /> </Tooltip>
