From ef8e9978d66196b298d158a641ffe4e4debd45a9 Mon Sep 17 00:00:00 2001 From: Enzo Martellucci Date: Thu, 11 Jun 2026 11:37:10 +0200 Subject: [PATCH] fix(chatbot): track view recency so last-loaded chatbot resolves correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The singleton chatbot resolver selects the most-recently-registered view, but the views registry backed each location with a Set and `add` is a no-op for an existing id — so re-registering a chatbot (e.g. on extension reload) did not move it to the end, and the resolver could keep selecting an older chatbot. - registerView now deletes-then-adds the id so re-registration reflects true recency; getRegisteredViewIds therefore returns ids in recency order. - Update the chatbot resolver comment to match (recency, not first-registration). - Add a registry regression test for re-register ordering. Also drop SIP section cross-references (§3.2, §10/§11) from the chatbot mount and dashboard context comments; the surrounding explanations stay. Co-Authored-By: Claude Opus 4.8 (1M context) --- superset-frontend/src/core/chatbot/index.ts | 9 +++--- superset-frontend/src/core/dashboard/index.ts | 4 +-- .../src/core/views/index.test.ts | 30 +++++++++++++++++++ superset-frontend/src/core/views/index.ts | 5 ++++ superset-frontend/src/views/App.tsx | 2 +- 5 files changed, 43 insertions(+), 7 deletions(-) diff --git a/superset-frontend/src/core/chatbot/index.ts b/superset-frontend/src/core/chatbot/index.ts index eded52c85a3..5a7f907e5d4 100644 --- a/superset-frontend/src/core/chatbot/index.ts +++ b/superset-frontend/src/core/chatbot/index.ts @@ -59,10 +59,11 @@ export const getActiveChatbot = (): ActiveChatbot | undefined => { return undefined; } - // `getRegisteredViewIds` returns ids in registration order, so the last entry - // is the most-recently-loaded chatbot. `getViewProvider` reads the same - // synchronous registry maps, so the id always has a live provider; the final - // guard is cheap defensiveness, not a fallback path. + // `getRegisteredViewIds` returns ids in recency order — the registry moves an + // id to the end when it re-registers — so the last entry is the + // most-recently-loaded chatbot. `getViewProvider` reads the same synchronous + // registry maps, so the id always has a live provider; the final guard is + // cheap defensiveness, not a fallback path. const selectedId = registeredIds[registeredIds.length - 1]; const provider = getViewProvider(CHATBOT_LOCATION, selectedId); diff --git a/superset-frontend/src/core/dashboard/index.ts b/superset-frontend/src/core/dashboard/index.ts index c912f845916..89afa1050c1 100644 --- a/superset-frontend/src/core/dashboard/index.ts +++ b/superset-frontend/src/core/dashboard/index.ts @@ -56,8 +56,8 @@ function buildChartSummaries(state: RootState): ChartSummary[] { vizType: slice?.viz_type ?? '', datasourceId: slice?.datasource_id ?? null, datasourceName: slice?.datasource_name ?? null, - // Tab-accurate visibility is a deferred phase (SIP §10/§11); every chart - // on the dashboard is reported visible for now. + // Tab-accurate visibility is a deferred phase; every chart on the + // dashboard is reported visible for now. isVisible: true, }; }); diff --git a/superset-frontend/src/core/views/index.test.ts b/superset-frontend/src/core/views/index.test.ts index 1112beb6491..c7f9d21ddea 100644 --- a/superset-frontend/src/core/views/index.test.ts +++ b/superset-frontend/src/core/views/index.test.ts @@ -168,6 +168,36 @@ test('getRegisteredViewIds returns ids in registration order', () => { ]); }); +test('re-registering an existing id moves it to the end (recency order)', () => { + const provider = () => React.createElement('div', null, 'Test'); + disposables.push( + views.registerView( + { id: 'first.chatbot', name: 'First' }, + 'core.chatbot', + provider, + ), + views.registerView( + { id: 'second.chatbot', name: 'Second' }, + 'core.chatbot', + provider, + ), + ); + + // Re-register the first id; it must become the most recent, not stay first. + disposables.push( + views.registerView( + { id: 'first.chatbot', name: 'First' }, + 'core.chatbot', + provider, + ), + ); + + expect(getRegisteredViewIds('core.chatbot')).toEqual([ + 'second.chatbot', + 'first.chatbot', + ]); +}); + test('getRegisteredViewIds returns an empty array for an unused location', () => { expect(getRegisteredViewIds('core.chatbot')).toEqual([]); }); diff --git a/superset-frontend/src/core/views/index.ts b/superset-frontend/src/core/views/index.ts index bc3bea3588f..3c15bf377f1 100644 --- a/superset-frontend/src/core/views/index.ts +++ b/superset-frontend/src/core/views/index.ts @@ -75,6 +75,11 @@ const registerView: typeof viewsApi.registerView = ( viewRegistry.set(id, { view, location, provider }); const ids = locationIndex.get(location) ?? new Set(); + // Re-registering an existing id must reflect its new recency. A Set preserves + // first-insertion order and `add` is a no-op for an existing member, so delete + // first to move the id to the end. Consumers that select the most-recently + // registered view (e.g. the chatbot resolver) depend on this ordering. + ids.delete(id); ids.add(id); locationIndex.set(location, ids); diff --git a/superset-frontend/src/views/App.tsx b/superset-frontend/src/views/App.tsx index d70aa0f86f9..0ea9014f575 100644 --- a/superset-frontend/src/views/App.tsx +++ b/superset-frontend/src/views/App.tsx @@ -118,7 +118,7 @@ const App = () => ( The singleton chatbot bubble. Rendered as a sibling of the route Switch — inside ExtensionsStartup so chatbot extensions have been loaded and registered, but outside the Switch so the bubble persists - across route changes (SIP §3.2). + across route changes. */} {isFeatureEnabled(FeatureFlag.EnableExtensions) && }