You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2026/06/18 16:25:03 UTC
[PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
michael-s-molina opened a new pull request, #41205:
URL: https://github.com/apache/superset/pull/41205
### SUMMARY
Implements the Chat contribution type described in [SIP-214](https://github.com/apache/superset/issues/41059), co-authored with Enzo Martellucci (@EnxDev). This introduces the `chat` and `navigation` namespaces in `@apache-superset/core`, giving extensions a first-class API to contribute a persistent chat interface and react to page navigation events. The host manages layout, open/close state, display mode, and persistence — extensions implement the trigger and panel components and use the API to drive behavior.
These are the first two namespaces in a planned series. Surface-specific namespaces (dashboard, explore, SQL Lab, dataset) will follow in subsequent SIPs, allowing extensions to progressively access deeper contextual information. Agentic UI actions — modifying charts, editing queries, triggering workflows — are intentionally out of scope here and deferred to the Client Actions SIP.
**Singleton model**
Only one chat is active at a time. This is a deliberate design choice: rendering multiple chat providers simultaneously would create competing conversational experiences and introduce ambiguity for the user. Chat is treated as a deployment-level selection rather than a multi-provider composition.
**Registering a chat**
```typescript
import { chat } from '@apache-superset/core';
import ChatTrigger from './ChatTrigger';
import ChatPanel from './ChatPanel';
chat.registerChat(
{ id: 'my-org.my-chat', name: 'My Chat' },
ChatTrigger,
ChatPanel,
);
```
The trigger is always visible in the bottom-right corner. The panel can be shown as a floating overlay or as a resizable sidebar docked beside the page content. The user's preference is persisted across reloads.
**Display modes**
- `'floating'` (default) — panel opens as an overlay anchored to the trigger.
- `'panel'` — the page layout splits into content + chat sidebar with a draggable divider.
Extensions can switch modes programmatically or react to user-initiated changes:
```typescript
chat.setDisplayMode('panel');
chat.onDidChangeDisplayMode(mode => {
console.log('display mode changed to', mode);
});
```
**Navigation awareness**
The `navigation` namespace is new in `@apache-superset/core`. Extensions can query the current page or subscribe to page changes, enabling context-aware chat behavior:
```typescript
import { navigation } from '@apache-superset/core';
console.log(navigation.getPage()); // e.g. 'dashboard', 'explore', 'sqllab'
navigation.onDidChangePage(page => {
console.log('navigated to', page);
});
```
**Documentation**
The Contribution Types page and a new Chat reference page (under `docs/developer_docs/extensions/`) cover registration, display modes, the full open/close and event API, and the navigation namespace.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://github.com/user-attachments/assets/9c76b583-0407-471f-8c07-e34736dcfeb7
### TESTING INSTRUCTIONS
**Manual verification**
Enable the `ENABLE_EXTENSIONS` feature flag, then create a minimal extension with the following `index.tsx`:
```tsx
import React from 'react';
import { chat } from '@apache-superset/core';
const Trigger = () => {
const handleClick = () =>
chat.isOpen() ? chat.close() : chat.open();
return <button onClick={handleClick}>💬</button>;
};
const Panel = () => (
<div style={{ background: '#fff', height: '100%', padding: 16 }}>
Hello from chat
</div>
);
chat.registerChat({ id: 'test.chat', name: 'Test Chat' }, Trigger, Panel);
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue
- [ ] Required feature flags: Chat host rendering is guarded by the existing `ENABLE_EXTENSIONS` flag
- [x] Changes UI
- [ ] Includes DB Migration
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41205:
URL: https://github.com/apache/superset/pull/41205#issuecomment-4746353599
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41205:
URL: https://github.com/apache/superset/pull/41205#issuecomment-4744023911
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3444197677
##########
superset-frontend/src/views/App.tsx:
##########
@@ -79,42 +84,137 @@ const LocationPathnameLogger = () => {
return <></>;
};
+const CHAT_PANEL_DEFAULT_WIDTH = 400;
+const CHAT_PANEL_MIN_WIDTH = 280;
+
+const RouteSwitch = () => (
+ <Switch>
+ {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
+ <Route path={path} key={path}>
+ <Suspense fallback={<Fallback />}>
+ <ErrorBoundary
+ css={css`
+ margin: 16px;
+ `}
+ >
+ <Component user={bootstrapData.user} {...props} />
+ </ErrorBoundary>
+ </Suspense>
+ </Route>
+ ))}
+ <Redirect from="/" to="/superset/welcome/" exact />
+ </Switch>
+);
+
+const layoutCss = css`
+ flex: 1;
+ min-height: 0;
+ overflow: hidden;
+`;
+
+const contentCss = css`
+ display: flex;
+ flex-direction: column;
+ min-height: 0;
+ overflow-y: auto;
+ position: relative;
+`;
+
+/**
+ * Renders the main content area. When the chat panel is open in panel mode,
+ * wraps <Layout> and <ChatPanelContent> in a Splitter so they sit side-by-side
+ * with a lazy drag bar (blue preview line, resize committed on mouseup).
+ * The full <Layout> tree lives inside the first panel so its internal flex
+ * context is preserved — SQL Lab, Explore, and other pages are unaffected.
+ */
+const AppContent = () => {
+ const isAuthenticated =
+ isUser(bootstrapData.user) && !bootstrapData.user.isAnonymous;
+ const chatExtensionsEnabled =
+ isFeatureEnabled(FeatureFlag.EnableExtensions) && isAuthenticated;
+ const { open: panelOpen, mode, chat } = useChat();
+ const hasChatExtension = chatExtensionsEnabled && !!chat;
+ const isPanelOpen = hasChatExtension && mode === 'panel' && panelOpen;
+
+ const [storedWidth, setStoredWidth] = useStoredSidebarWidth(
+ 'chat:panel',
+ CHAT_PANEL_DEFAULT_WIDTH,
+ );
+
+ const layoutContent = (
+ <Layout css={layoutCss}>
+ <Layout.Content css={contentCss}>
+ <RouteSwitch />
+ </Layout.Content>
+ </Layout>
+ );
+
+ if (!isPanelOpen) {
+ return (
+ <>
+ {layoutContent}
+ {hasChatExtension && <ChatFloatingHost />}
+ </>
+ );
+ }
+
+ return (
+ <Splitter
+ lazy
+ onResizeEnd={sizes => {
+ const chatWidth = sizes[sizes.length - 1];
+ if (
+ typeof chatWidth === 'number' &&
+ chatWidth >= CHAT_PANEL_MIN_WIDTH
+ ) {
+ setStoredWidth(chatWidth);
+ }
+ }}
+ css={css`
+ flex: 1;
+ min-height: 0;
+ overflow: hidden;
+
+ /*
+ * Splitter.Panel is not a flex container by default, so flex:1 on
+ * children (Layout, ChatPanelHost) has no height effect and
+ * panels collapse. Making them flex columns lets children fill them.
+ */
+ & > .ant-splitter-panel {
+ display: flex !important;
+ flex-direction: column;
+ }
+ `}
+ >
+ <Splitter.Panel>{layoutContent}</Splitter.Panel>
+ <Splitter.Panel size={storedWidth} min={CHAT_PANEL_MIN_WIDTH}>
+ <ChatPanelHost />
+ </Splitter.Panel>
+ </Splitter>
+ );
+};
+
const App = () => (
<Router basename={applicationRoot()}>
<ScrollToTop />
<LocationPathnameLogger />
<RootContextProviders>
- <Menu
- data={bootstrapData.common.menu_data}
- isFrontendRoute={isFrontendRoute}
- />
- <ExtensionsStartup>
- <Switch>
- {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
- <Route path={path} key={path}>
- <Suspense fallback={<Fallback />}>
- <Layout>
- <Layout.Content
- css={css`
- display: flex;
- flex-direction: column;
- `}
- >
- <ErrorBoundary
- css={css`
- margin: 16px;
- `}
- >
- <Component user={bootstrapData.user} {...props} />
- </ErrorBoundary>
- </Layout.Content>
- </Layout>
- </Suspense>
- </Route>
- ))}
- <Redirect from="/" to="/superset/welcome/" exact />
- </Switch>
- </ExtensionsStartup>
+ <div
+ css={css`
+ display: flex;
+ flex-direction: column;
+ height: 100vh;
+ overflow: hidden;
+ `}
+ >
Review Comment:
Honestly, not much in this specific case. Since we still need a css prop for `height: 100vh; overflow: hidden`, we're not eliminating CSS — just moving `display: flex; flex-direction: column` into the component prop. The result is arguably the same verbosity.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41205:
URL: https://github.com/apache/superset/pull/41205#issuecomment-4744038827
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | bc3d0af77aa1d8871b30493d763905e787e6dbdc |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a341bdf7c79e100076dbee6 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41205--superset-docs-preview.netlify.app](https://deploy-preview-41205--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEADAv-y5QCmU-kHPcNcNhdJFtoNEf8_bHOYFafUWhR1gAmyhUBqmFHSmWrRlfDTXMmsXHiOyXLUfcRwXltViPD35ja0xxruIzA6-Hw) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41205--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41205:
URL: https://github.com/apache/superset/pull/41205#issuecomment-4745093672
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41205?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 55.59%. Comparing base ([`74ae5a4`](https://app.codecov.io/gh/apache/superset/commit/74ae5a45f9ae25887ec07094680e784d48385662?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1847608`](https://app.codecov.io/gh/apache/superset/commit/1847608053a0e0e2c43ade6e33a001084381bca8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41205 +/- ##
==========================================
- Coverage 64.33% 55.59% -8.74%
==========================================
Files 2651 2651
Lines 144766 144710 -56
Branches 33401 33403 +2
==========================================
- Hits 93131 80454 -12677
- Misses 49965 63523 +13558
+ Partials 1670 733 -937
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41205/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.90% <ø> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/41205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.90% <ø> (-18.67%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/41205/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41205?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3437440708
##########
superset-frontend/src/core/editors/EditorProviders.ts:
##########
@@ -83,15 +47,9 @@ class EditorProviders {
*/
private languageToProvider: Map<EditorLanguage, string> = new Map();
- /**
- * Event emitter for provider registration events.
- */
- private registerEmitter = new EventEmitter<EditorRegisteredEvent>();
+ private registerEmitter = createEventEmitter<EditorRegisteredEvent>();
- /**
- * Event emitter for provider unregistration events.
- */
- private unregisterEmitter = new EventEmitter<EditorUnregisteredEvent>();
+ private unregisterEmitter = createEventEmitter<EditorUnregisteredEvent>();
Review Comment:
**Suggestion:** These emitter instances are long-lived singleton fields, but `reset()` does not clear or recreate them, so listeners registered in prior test/runtime phases remain retained and can fire unexpectedly later. Reinitialize emitters (or add clear support) during reset to avoid stale listener retention. [memory leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ EditorProviders tests can leak listeners between cases.
- ⚠️ Future tests using registration events risk flaky behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset-frontend/src/core/utils.ts:44-54`, `createEventEmitter<T>()` creates a
closure over a `listeners` `Set` and returns `{ fire, subscribe }`, with `subscribe`
adding listeners to that `Set` but no way to clear it.
2. In `superset-frontend/src/core/editors/EditorProviders.ts:50-52`, `EditorProviders`
constructs `registerEmitter` and `unregisterEmitter` once as instance fields using
`createEventEmitter`, so their internal `listeners` sets are long-lived for the lifetime
of the singleton.
3. In the same file, `onDidRegister()` and `onDidUnregister()` at
`EditorProviders.ts:187-199` invoke `this.registerEmitter.subscribe(listener)` /
`this.unregisterEmitter.subscribe(listener)` and return the resulting `Disposable`, but
`reset()` at `EditorProviders.ts:205-209` only clears `providers`, `languageToProvider`,
and `syncListeners` — it does not recreate the emitters or clear their internal
`listeners` sets.
4. As shown in `superset-frontend/src/core/editors/EditorProviders.test.ts:7-11`, tests
call `EditorProviders.getInstance().reset()` in `beforeEach`; if a test registers
listeners via `onDidRegister()` or `onDidUnregister()`, those listeners will remain in the
emitter's internal `Set` after `reset()`, and subsequent tests that register providers
(calling `registerProvider()` at `EditorProviders.ts:89-123`, which fires
`registerEmitter.fire(...)` at line 116) will still invoke the stale listeners,
demonstrating the listener leak across resets.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=247bf606834e4d189ba95038a43e15d3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=247bf606834e4d189ba95038a43e15d3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/editors/EditorProviders.ts
**Line:** 50:52
**Comment:**
*Memory Leak: These emitter instances are long-lived singleton fields, but `reset()` does not clear or recreate them, so listeners registered in prior test/runtime phases remain retained and can fire unexpectedly later. Reinitialize emitters (or add clear support) during reset to avoid stale listener retention.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=e2680bfb015b270190bbf1a849f9ad779a16198f826b38e4d4898035f311f828&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=e2680bfb015b270190bbf1a849f9ad779a16198f826b38e4d4898035f311f828&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/editors/index.ts:
##########
@@ -18,130 +18,39 @@
*/
/**
- * @fileoverview Implementation of the editors API for Superset.
+ * @fileoverview Host implementation of the `editors` contribution type.
*
- * This module provides the runtime implementation of the editor registration
- * and resolution functions declared in the API types.
+ * Extensions register via the public `editors.registerEditor()` and the host
+ * resolves the appropriate provider per language, falling back to the built-in
+ * AceEditorProvider when no extension is registered.
+ *
+ * The public namespace (`editors`) is exposed to extensions on `window.superset`.
+ * `EditorHost` is the host-internal component for rendering editors and is NOT
+ * part of the public `@apache-superset/core` API.
*/
import { useSyncExternalStore } from 'react';
import { editors as editorsApi } from '@apache-superset/core';
-import { Disposable } from '../models';
import EditorProviders from './EditorProviders';
-type EditorLanguage = editorsApi.EditorLanguage;
-type Editor = editorsApi.Editor;
-type EditorProvider = editorsApi.EditorProvider;
-type EditorComponent = editorsApi.EditorComponent;
-type EditorRegisteredEvent = editorsApi.EditorRegisteredEvent;
-type EditorUnregisteredEvent = editorsApi.EditorUnregisteredEvent;
-
-/**
- * Register an editor provider as a module-level side effect.
- * Takes the editor descriptor directly rather than looking it up
- * from a manifest by ID.
- *
- * @param editor The editor descriptor.
- * @param component The React component implementing the editor.
- * @returns A Disposable to unregister the provider.
- */
-export const registerEditor = (
- editor: Editor,
- component: EditorComponent,
-): Disposable => {
- const providers = EditorProviders.getInstance();
- return providers.registerProvider(editor, component);
-};
-
-/**
- * Get the editor provider for a specific language.
- * Returns the extension's editor if registered, otherwise undefined.
- *
- * @param language The language to get an editor for
- * @returns The editor provider or undefined if no extension provides one
- */
-export const getEditor = (
- language: EditorLanguage,
-): EditorProvider | undefined => {
- const manager = EditorProviders.getInstance();
- return manager.getProvider(language);
-};
-
-/**
- * Check if an extension has registered an editor for a language.
- *
- * @param language The language to check
- * @returns True if an extension provides an editor for this language
- */
-export const hasEditor = (language: EditorLanguage): boolean => {
- const manager = EditorProviders.getInstance();
- return manager.hasProvider(language);
-};
-
-/**
- * Get all registered editor providers.
- *
- * @returns Array of all registered editor providers
- */
-export const getAllEditors = (): EditorProvider[] => {
- const manager = EditorProviders.getInstance();
- return manager.getAllProviders();
-};
-
-/**
- * Event fired when an editor is registered.
- * Subscribe to this event to react when extensions register new editors.
- */
-export const onDidRegisterEditor = (
- listener: (e: EditorRegisteredEvent) => void,
-): Disposable => {
- const manager = EditorProviders.getInstance();
- return manager.onDidRegister(listener);
-};
+export type { EditorHostProps } from './EditorHost';
+export { default as EditorHost } from './EditorHost';
+export { default as AceEditorProvider } from './AceEditorProvider';
-/**
- * Event fired when an editor is unregistered.
- * Subscribe to this event to react when extensions unregister editors.
- */
-export const onDidUnregisterEditor = (
- listener: (e: EditorUnregisteredEvent) => void,
-): Disposable => {
- const manager = EditorProviders.getInstance();
- return manager.onDidUnregister(listener);
-};
+const provider = EditorProviders.getInstance();
-/**
- * Hook that returns the editor provider for a specific language and re-renders when it changes.
- *
- * @param language The language to get an editor for
- * @returns The editor provider or undefined if no extension provides one
- */
-export const useEditor = (
- language: EditorLanguage,
-): EditorProvider | undefined => {
- const manager = EditorProviders.getInstance();
- return useSyncExternalStore(
- manager.subscribe,
- () => manager.getProvider(language),
+export const useEditor = (language: editorsApi.EditorLanguage) =>
+ useSyncExternalStore(
+ provider.subscribe,
+ () => provider.getProvider(language),
() => undefined,
);
-};
-/**
- * Editors API object for use in the extension system.
- */
export const editors: typeof editorsApi = {
- registerEditor,
- getEditor,
- hasEditor,
- getAllEditors,
- onDidRegisterEditor,
- onDidUnregisterEditor,
+ registerEditor: provider.registerProvider.bind(provider),
+ getEditor: provider.getProvider.bind(provider),
+ hasEditor: provider.hasProvider.bind(provider),
+ getAllEditors: provider.getAllProviders.bind(provider),
+ onDidRegisterEditor: provider.onDidRegister.bind(provider),
+ onDidUnregisterEditor: provider.onDidUnregister.bind(provider),
Review Comment:
**Suggestion:** This has the same contract break for unregister events: the optional `thisArgs` from the `Event` API is ignored, so context-bound listeners won't behave correctly. Add a wrapper that accepts and forwards both listener and context. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Extensions listening for editor unregistration lose bound context.
- ⚠️ Breaks symmetry with other Event<T>-based APIs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The public API in
`superset-frontend/packages/superset-core/src/editors/index.ts:618-621` declares
`onDidUnregisterEditor` as `Event<EditorUnregisteredEvent>`, where `Event<T>` (from
`packages/superset-core/src/common/index.ts:35-40,14-15`) supports an optional `thisArgs`
parameter for binding listener context.
2. The host implementation in `superset-frontend/src/core/editors/index.ts:49-56` wires
`onDidUnregisterEditor` as `provider.onDidUnregister.bind(provider)`, mirroring the
register event wiring.
3. Inside `superset-frontend/src/core/editors/EditorProviders.ts:192-199`,
`onDidUnregister` is defined as `public onDidUnregister(listener:
Listener<EditorUnregisteredEvent>): Disposable { return
this.unregisterEmitter.subscribe(listener); }`, only accepting `listener` and not
propagating any `thisArgs` value to `subscribe`.
4. If extension code subscribes using `editors.onDidUnregisterEditor(function (e) {
this.handleCleanup(e); }, myContext)`, the implementation discards `myContext` and
`createEventEmitter.subscribe` (see `src/core/utils.ts:44-49`) binds the raw listener
without a `this` context; when `unregisterProvider()` fires the event at
`EditorProviders.ts:148`, `this` inside the listener is `undefined`, breaking any listener
that expects the documented context binding.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=48d9db4e30864b8ba132d86577a27d7e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=48d9db4e30864b8ba132d86577a27d7e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/editors/index.ts
**Line:** 55:55
**Comment:**
*Api Mismatch: This has the same contract break for unregister events: the optional `thisArgs` from the `Event` API is ignored, so context-bound listeners won't behave correctly. Add a wrapper that accepts and forwards both listener and context.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=2c4429d957254e8509fc410fc2a6c8453e62400938112751a4dc31e1520d22f6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=2c4429d957254e8509fc410fc2a6c8453e62400938112751a4dc31e1520d22f6&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/editors/index.ts:
##########
@@ -18,130 +18,39 @@
*/
/**
- * @fileoverview Implementation of the editors API for Superset.
+ * @fileoverview Host implementation of the `editors` contribution type.
*
- * This module provides the runtime implementation of the editor registration
- * and resolution functions declared in the API types.
+ * Extensions register via the public `editors.registerEditor()` and the host
+ * resolves the appropriate provider per language, falling back to the built-in
+ * AceEditorProvider when no extension is registered.
+ *
+ * The public namespace (`editors`) is exposed to extensions on `window.superset`.
+ * `EditorHost` is the host-internal component for rendering editors and is NOT
+ * part of the public `@apache-superset/core` API.
*/
import { useSyncExternalStore } from 'react';
import { editors as editorsApi } from '@apache-superset/core';
-import { Disposable } from '../models';
import EditorProviders from './EditorProviders';
-type EditorLanguage = editorsApi.EditorLanguage;
-type Editor = editorsApi.Editor;
-type EditorProvider = editorsApi.EditorProvider;
-type EditorComponent = editorsApi.EditorComponent;
-type EditorRegisteredEvent = editorsApi.EditorRegisteredEvent;
-type EditorUnregisteredEvent = editorsApi.EditorUnregisteredEvent;
-
-/**
- * Register an editor provider as a module-level side effect.
- * Takes the editor descriptor directly rather than looking it up
- * from a manifest by ID.
- *
- * @param editor The editor descriptor.
- * @param component The React component implementing the editor.
- * @returns A Disposable to unregister the provider.
- */
-export const registerEditor = (
- editor: Editor,
- component: EditorComponent,
-): Disposable => {
- const providers = EditorProviders.getInstance();
- return providers.registerProvider(editor, component);
-};
-
-/**
- * Get the editor provider for a specific language.
- * Returns the extension's editor if registered, otherwise undefined.
- *
- * @param language The language to get an editor for
- * @returns The editor provider or undefined if no extension provides one
- */
-export const getEditor = (
- language: EditorLanguage,
-): EditorProvider | undefined => {
- const manager = EditorProviders.getInstance();
- return manager.getProvider(language);
-};
-
-/**
- * Check if an extension has registered an editor for a language.
- *
- * @param language The language to check
- * @returns True if an extension provides an editor for this language
- */
-export const hasEditor = (language: EditorLanguage): boolean => {
- const manager = EditorProviders.getInstance();
- return manager.hasProvider(language);
-};
-
-/**
- * Get all registered editor providers.
- *
- * @returns Array of all registered editor providers
- */
-export const getAllEditors = (): EditorProvider[] => {
- const manager = EditorProviders.getInstance();
- return manager.getAllProviders();
-};
-
-/**
- * Event fired when an editor is registered.
- * Subscribe to this event to react when extensions register new editors.
- */
-export const onDidRegisterEditor = (
- listener: (e: EditorRegisteredEvent) => void,
-): Disposable => {
- const manager = EditorProviders.getInstance();
- return manager.onDidRegister(listener);
-};
+export type { EditorHostProps } from './EditorHost';
+export { default as EditorHost } from './EditorHost';
+export { default as AceEditorProvider } from './AceEditorProvider';
-/**
- * Event fired when an editor is unregistered.
- * Subscribe to this event to react when extensions unregister editors.
- */
-export const onDidUnregisterEditor = (
- listener: (e: EditorUnregisteredEvent) => void,
-): Disposable => {
- const manager = EditorProviders.getInstance();
- return manager.onDidUnregister(listener);
-};
+const provider = EditorProviders.getInstance();
-/**
- * Hook that returns the editor provider for a specific language and re-renders when it changes.
- *
- * @param language The language to get an editor for
- * @returns The editor provider or undefined if no extension provides one
- */
-export const useEditor = (
- language: EditorLanguage,
-): EditorProvider | undefined => {
- const manager = EditorProviders.getInstance();
- return useSyncExternalStore(
- manager.subscribe,
- () => manager.getProvider(language),
+export const useEditor = (language: editorsApi.EditorLanguage) =>
+ useSyncExternalStore(
+ provider.subscribe,
+ () => provider.getProvider(language),
() => undefined,
);
-};
-/**
- * Editors API object for use in the extension system.
- */
export const editors: typeof editorsApi = {
- registerEditor,
- getEditor,
- hasEditor,
- getAllEditors,
- onDidRegisterEditor,
- onDidUnregisterEditor,
+ registerEditor: provider.registerProvider.bind(provider),
+ getEditor: provider.getProvider.bind(provider),
+ hasEditor: provider.hasProvider.bind(provider),
+ getAllEditors: provider.getAllProviders.bind(provider),
+ onDidRegisterEditor: provider.onDidRegister.bind(provider),
Review Comment:
**Suggestion:** This wiring drops the `thisArgs` part of the `Event` contract because `onDidRegister` only forwards the listener and ignores the optional context argument. Callers using `editors.onDidRegisterEditor(listener, ctx)` will get an unbound callback and `this`-dependent listeners will break. Expose a wrapper matching the full `Event` signature and forward `thisArgs`. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Extensions using thisArgs on editor registration will break.
- ⚠️ Violates Event<T> contract advertised in superset-core.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The public editors API in
`superset-frontend/packages/superset-core/src/editors/index.ts:610-617` declares
`onDidRegisterEditor` as `Event<EditorRegisteredEvent>`, and the `Event<T>` type in
`superset-frontend/packages/superset-core/src/common/index.ts:35-40,14-15` explicitly has
signature `(listener: (e: T) => any, thisArgs?: any): Disposable`, supporting an optional
`thisArgs` context.
2. The host implementation wires this in
`superset-frontend/src/core/editors/index.ts:49-56` as `export const editors: typeof
editorsApi = { ..., onDidRegisterEditor: provider.onDidRegister.bind(provider), ... }`,
where `provider` is the singleton from `EditorProviders.getInstance()` at line 40.
3. `EditorProviders.onDidRegister` in
`superset-frontend/src/core/editors/EditorProviders.ts:187-189` is defined as `public
onDidRegister(listener: Listener<EditorRegisteredEvent>): Disposable { return
this.registerEmitter.subscribe(listener); }`, which only accepts `listener` and does not
accept or forward the optional `thisArgs` parameter.
4. An extension using the documented API via `window.superset.editors` (populated in
`superset-frontend/src/extensions/ExtensionsStartup.tsx:38-54`) can legitimately call
`editors.onDidRegisterEditor(function (e) { this.handle(e); }, myContext)`, but because
the implementation is the bound method `provider.onDidRegister` that ignores the second
argument, `createEventEmitter.subscribe` (in `src/core/utils.ts:44-49`) never receives
`thisArgs`, so the listener is invoked with `this === undefined` when `registerProvider()`
fires the event at `EditorProviders.ts:116`. Any listener relying on `this` will then
throw (e.g., `TypeError: Cannot read properties of undefined`), violating the `Event`
contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=83e9d5506a894404838c1940202a8b48&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=83e9d5506a894404838c1940202a8b48&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/editors/index.ts
**Line:** 54:54
**Comment:**
*Api Mismatch: This wiring drops the `thisArgs` part of the `Event` contract because `onDidRegister` only forwards the listener and ignores the optional context argument. Callers using `editors.onDidRegisterEditor(listener, ctx)` will get an unbound callback and `this`-dependent listeners will break. Expose a wrapper matching the full `Event` signature and forward `thisArgs`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=098e7e96eb55ae6c8ebc5efa0b4953602384932f4ecac32b354b933b02c05cbb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=098e7e96eb55ae6c8ebc5efa0b4953602384932f4ecac32b354b933b02c05cbb&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3444197677
##########
superset-frontend/src/views/App.tsx:
##########
@@ -79,42 +84,137 @@ const LocationPathnameLogger = () => {
return <></>;
};
+const CHAT_PANEL_DEFAULT_WIDTH = 400;
+const CHAT_PANEL_MIN_WIDTH = 280;
+
+const RouteSwitch = () => (
+ <Switch>
+ {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
+ <Route path={path} key={path}>
+ <Suspense fallback={<Fallback />}>
+ <ErrorBoundary
+ css={css`
+ margin: 16px;
+ `}
+ >
+ <Component user={bootstrapData.user} {...props} />
+ </ErrorBoundary>
+ </Suspense>
+ </Route>
+ ))}
+ <Redirect from="/" to="/superset/welcome/" exact />
+ </Switch>
+);
+
+const layoutCss = css`
+ flex: 1;
+ min-height: 0;
+ overflow: hidden;
+`;
+
+const contentCss = css`
+ display: flex;
+ flex-direction: column;
+ min-height: 0;
+ overflow-y: auto;
+ position: relative;
+`;
+
+/**
+ * Renders the main content area. When the chat panel is open in panel mode,
+ * wraps <Layout> and <ChatPanelContent> in a Splitter so they sit side-by-side
+ * with a lazy drag bar (blue preview line, resize committed on mouseup).
+ * The full <Layout> tree lives inside the first panel so its internal flex
+ * context is preserved — SQL Lab, Explore, and other pages are unaffected.
+ */
+const AppContent = () => {
+ const isAuthenticated =
+ isUser(bootstrapData.user) && !bootstrapData.user.isAnonymous;
+ const chatExtensionsEnabled =
+ isFeatureEnabled(FeatureFlag.EnableExtensions) && isAuthenticated;
+ const { open: panelOpen, mode, chat } = useChat();
+ const hasChatExtension = chatExtensionsEnabled && !!chat;
+ const isPanelOpen = hasChatExtension && mode === 'panel' && panelOpen;
+
+ const [storedWidth, setStoredWidth] = useStoredSidebarWidth(
+ 'chat:panel',
+ CHAT_PANEL_DEFAULT_WIDTH,
+ );
+
+ const layoutContent = (
+ <Layout css={layoutCss}>
+ <Layout.Content css={contentCss}>
+ <RouteSwitch />
+ </Layout.Content>
+ </Layout>
+ );
+
+ if (!isPanelOpen) {
+ return (
+ <>
+ {layoutContent}
+ {hasChatExtension && <ChatFloatingHost />}
+ </>
+ );
+ }
+
+ return (
+ <Splitter
+ lazy
+ onResizeEnd={sizes => {
+ const chatWidth = sizes[sizes.length - 1];
+ if (
+ typeof chatWidth === 'number' &&
+ chatWidth >= CHAT_PANEL_MIN_WIDTH
+ ) {
+ setStoredWidth(chatWidth);
+ }
+ }}
+ css={css`
+ flex: 1;
+ min-height: 0;
+ overflow: hidden;
+
+ /*
+ * Splitter.Panel is not a flex container by default, so flex:1 on
+ * children (Layout, ChatPanelHost) has no height effect and
+ * panels collapse. Making them flex columns lets children fill them.
+ */
+ & > .ant-splitter-panel {
+ display: flex !important;
+ flex-direction: column;
+ }
+ `}
+ >
+ <Splitter.Panel>{layoutContent}</Splitter.Panel>
+ <Splitter.Panel size={storedWidth} min={CHAT_PANEL_MIN_WIDTH}>
+ <ChatPanelHost />
+ </Splitter.Panel>
+ </Splitter>
+ );
+};
+
const App = () => (
<Router basename={applicationRoot()}>
<ScrollToTop />
<LocationPathnameLogger />
<RootContextProviders>
- <Menu
- data={bootstrapData.common.menu_data}
- isFrontendRoute={isFrontendRoute}
- />
- <ExtensionsStartup>
- <Switch>
- {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
- <Route path={path} key={path}>
- <Suspense fallback={<Fallback />}>
- <Layout>
- <Layout.Content
- css={css`
- display: flex;
- flex-direction: column;
- `}
- >
- <ErrorBoundary
- css={css`
- margin: 16px;
- `}
- >
- <Component user={bootstrapData.user} {...props} />
- </ErrorBoundary>
- </Layout.Content>
- </Layout>
- </Suspense>
- </Route>
- ))}
- <Redirect from="/" to="/superset/welcome/" exact />
- </Switch>
- </ExtensionsStartup>
+ <div
+ css={css`
+ display: flex;
+ flex-direction: column;
+ height: 100vh;
+ overflow: hidden;
+ `}
+ >
Review Comment:
Honestly, not much in this specific case. Since we still need a css prop for `height: 100vh; overflow: hidden`, we're not eliminating CSS — just moving `display: flex; flex-direction: column` into the component prop. The result is arguably the same verbosity.
I'll change it anyway as it's more similar to other pages.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #41205:
URL: https://github.com/apache/superset/pull/41205
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3439067030
##########
superset-frontend/src/views/App.tsx:
##########
@@ -79,42 +84,137 @@ const LocationPathnameLogger = () => {
return <></>;
};
+const CHAT_PANEL_DEFAULT_WIDTH = 400;
+const CHAT_PANEL_MIN_WIDTH = 280;
+
+const RouteSwitch = () => (
+ <Switch>
+ {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
+ <Route path={path} key={path}>
+ <Suspense fallback={<Fallback />}>
+ <ErrorBoundary
+ css={css`
+ margin: 16px;
Review Comment:
Wath do you think to use the Antd Token here? Like `theme.fontSizeLG`
##########
superset-frontend/src/views/App.tsx:
##########
@@ -79,42 +84,137 @@ const LocationPathnameLogger = () => {
return <></>;
};
+const CHAT_PANEL_DEFAULT_WIDTH = 400;
+const CHAT_PANEL_MIN_WIDTH = 280;
+
+const RouteSwitch = () => (
+ <Switch>
+ {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
+ <Route path={path} key={path}>
+ <Suspense fallback={<Fallback />}>
+ <ErrorBoundary
+ css={css`
+ margin: 16px;
+ `}
+ >
+ <Component user={bootstrapData.user} {...props} />
+ </ErrorBoundary>
+ </Suspense>
+ </Route>
+ ))}
+ <Redirect from="/" to="/superset/welcome/" exact />
+ </Switch>
+);
+
+const layoutCss = css`
+ flex: 1;
+ min-height: 0;
+ overflow: hidden;
+`;
+
+const contentCss = css`
+ display: flex;
+ flex-direction: column;
+ min-height: 0;
+ overflow-y: auto;
+ position: relative;
+`;
+
+/**
+ * Renders the main content area. When the chat panel is open in panel mode,
+ * wraps <Layout> and <ChatPanelContent> in a Splitter so they sit side-by-side
+ * with a lazy drag bar (blue preview line, resize committed on mouseup).
+ * The full <Layout> tree lives inside the first panel so its internal flex
+ * context is preserved — SQL Lab, Explore, and other pages are unaffected.
+ */
+const AppContent = () => {
+ const isAuthenticated =
+ isUser(bootstrapData.user) && !bootstrapData.user.isAnonymous;
+ const chatExtensionsEnabled =
+ isFeatureEnabled(FeatureFlag.EnableExtensions) && isAuthenticated;
+ const { open: panelOpen, mode, chat } = useChat();
+ const hasChatExtension = chatExtensionsEnabled && !!chat;
+ const isPanelOpen = hasChatExtension && mode === 'panel' && panelOpen;
+
+ const [storedWidth, setStoredWidth] = useStoredSidebarWidth(
+ 'chat:panel',
+ CHAT_PANEL_DEFAULT_WIDTH,
+ );
+
+ const layoutContent = (
+ <Layout css={layoutCss}>
+ <Layout.Content css={contentCss}>
+ <RouteSwitch />
+ </Layout.Content>
+ </Layout>
+ );
+
+ if (!isPanelOpen) {
+ return (
+ <>
+ {layoutContent}
+ {hasChatExtension && <ChatFloatingHost />}
+ </>
+ );
+ }
+
+ return (
+ <Splitter
+ lazy
+ onResizeEnd={sizes => {
+ const chatWidth = sizes[sizes.length - 1];
+ if (
+ typeof chatWidth === 'number' &&
+ chatWidth >= CHAT_PANEL_MIN_WIDTH
+ ) {
+ setStoredWidth(chatWidth);
+ }
+ }}
+ css={css`
+ flex: 1;
+ min-height: 0;
+ overflow: hidden;
+
+ /*
+ * Splitter.Panel is not a flex container by default, so flex:1 on
+ * children (Layout, ChatPanelHost) has no height effect and
+ * panels collapse. Making them flex columns lets children fill them.
+ */
+ & > .ant-splitter-panel {
+ display: flex !important;
+ flex-direction: column;
+ }
+ `}
+ >
+ <Splitter.Panel>{layoutContent}</Splitter.Panel>
+ <Splitter.Panel size={storedWidth} min={CHAT_PANEL_MIN_WIDTH}>
+ <ChatPanelHost />
+ </Splitter.Panel>
+ </Splitter>
+ );
+};
+
const App = () => (
<Router basename={applicationRoot()}>
<ScrollToTop />
<LocationPathnameLogger />
<RootContextProviders>
- <Menu
- data={bootstrapData.common.menu_data}
- isFrontendRoute={isFrontendRoute}
- />
- <ExtensionsStartup>
- <Switch>
- {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
- <Route path={path} key={path}>
- <Suspense fallback={<Fallback />}>
- <Layout>
- <Layout.Content
- css={css`
- display: flex;
- flex-direction: column;
- `}
- >
- <ErrorBoundary
- css={css`
- margin: 16px;
- `}
- >
- <Component user={bootstrapData.user} {...props} />
- </ErrorBoundary>
- </Layout.Content>
- </Layout>
- </Suspense>
- </Route>
- ))}
- <Redirect from="/" to="/superset/welcome/" exact />
- </Switch>
- </ExtensionsStartup>
+ <div
+ css={css`
+ display: flex;
+ flex-direction: column;
+ height: 100vh;
+ overflow: hidden;
+ `}
+ >
Review Comment:
Do you think it would make sense to replace this with the Ant Design `<Flex>` component here?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41205:
URL: https://github.com/apache/superset/pull/41205#issuecomment-4755093827
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3442151988
##########
docs/developer_docs/extensions/extension-points/chat.md:
##########
@@ -0,0 +1,140 @@
+---
+title: Chat
+sidebar_position: 3
+---
+
+<!--
+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.
+-->
+
+# Chat Contributions
+
+Extensions can add a chat interface to Superset by registering a trigger and a panel. The host owns the layout, open/close state, and display mode — the extension only needs to provide the UI components.
+
+## Overview
+
+A chat registration consists of two React components:
+
+| Component | Role |
+|-----------|------|
+| **Trigger** | Always-visible entry point (e.g., a floating button). Rendered in the bottom-right corner in floating mode, or as a fixed overlay in panel mode. |
+| **Panel** | The chat UI itself (message list, input, etc.). Mounted by the host in the active display mode. |
+
+## Display Modes
+
+The host supports two display modes, switchable by the user or the extension at runtime:
+
+| Mode | Behavior |
+|------|----------|
+| `floating` | Panel floats above page content, anchored to the bottom-right corner. |
+| `panel` | Panel is docked to the right side of the application as a resizable sidebar, sitting beside the page content. |
+
+The user's last selected mode and open/closed state are persisted across page reloads.
+
+## Registering a Chat
+
+Call `chat.registerChat` from your extension's entry point with a descriptor, a trigger factory, and a panel factory:
+
+```tsx
+import { chat } from '@apache-superset/core';
+import ChatTrigger from './ChatTrigger';
+import ChatPanel from './ChatPanel';
+
+chat.registerChat(
+ { id: 'my-org.my-chat', name: 'My Chat' },
+ ChatTrigger,
+ ChatPanel,
+);
+```
+
+Only one chat registration is active at a time. If a second extension calls `registerChat`, it replaces the first and a warning is logged.
+
+## Opening and Closing the Chat
+
+The trigger component is responsible for toggling the panel. Use `chat.isOpen()`, `chat.open()`, and `chat.close()` to control visibility:
+
+```tsx
+import { chat } from '@apache-superset/core';
+
+export default function ChatTrigger() {
+ return (
+ <button onClick={() => (chat.isOpen() ? chat.close() : chat.open())}>
+ 💬
+ </button>
+ );
+}
+```
+
+You can also subscribe to open/close events from any component:
+
+```tsx
+useEffect(() => {
+ const { dispose } = chat.onDidOpen(() => console.log('chat opened'));
+ return dispose;
+}, []);
+```
+
+## Changing the Display Mode
+
+Call `chat.setDisplayMode` to switch between `'floating'` and `'panel'` modes. In your panel component, subscribe to `onDidChangeDisplayMode` to react to changes (including those triggered by the user):
+
+```tsx
+import { useState, useEffect } from 'react';
+import { chat } from '@apache-superset/core';
+
+export default function ChatPanel() {
+ const [mode, setMode] = useState(chat.getDisplayMode());
+
+ useEffect(() => {
+ const { dispose } = chat.onDidChangeDisplayMode(m => setMode(m));
+ return dispose;
+ }, []);
+
+ return (
+ <div style={{ height: mode === 'panel' ? '100%' : '80vh' }}>
+ <button onClick={() => chat.setDisplayMode(mode === 'panel' ? 'floating' : 'panel')}>
+ {mode === 'panel' ? 'Float' : 'Dock'}
+ </button>
+ {/* message list and input */}
+ </div>
+ );
+}
+```
+
+## Chat API Reference
+
+All methods are available on the `chat` namespace from `@apache-superset/core`:
+
+| Method / Event | Description |
+|----------------|-------------|
+| `registerChat(descriptor, trigger, panel)` | Register a chat extension. Returns a `Disposable` to unregister. |
+| `open()` | Open the chat panel. No-op if already open or no registration. |
+| `close()` | Close the chat panel. |
+| `isOpen()` | Returns `true` if the panel is currently open. |
+| `getDisplayMode()` | Returns the current display mode (`'floating'` or `'panel'`). |
+| `setDisplayMode(mode)` | Switch between `'floating'` and `'panel'` mode. |
+| `onDidOpen(listener)` | Subscribe to panel open events. Returns a `Disposable`. |
+| `onDidClose(listener)` | Subscribe to panel close events. Returns a `Disposable`. |
+| `onDidChangeDisplayMode(listener)` | Subscribe to display mode changes. Returns a `Disposable`. |
+| `onDidRegisterChat(listener)` | Subscribe to registration events. |
+| `onDidUnregisterChat(listener)` | Subscribe to unregistration events. |
+
Review Comment:
Good point! Will add.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3444206073
##########
superset-frontend/src/core/navigation/index.ts:
##########
@@ -0,0 +1,94 @@
+/**
+ * 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.
+ */
+
+/**
+ * @fileoverview Host-internal implementation of the `navigation` namespace.
+ *
+ * Derives the current {@link Page} from the browser location by matching
+ * against {@link RoutePaths}. Call {@link useNavigationTracker} once in the
+ * app shell to keep the page in sync with React Router.
+ */
+
+import { useEffect, useRef } from 'react';
+import { useLocation, matchPath } from 'react-router-dom';
+import type { navigation as navigationApi } from '@apache-superset/core';
+import { RoutePaths } from '../../views/routePaths';
+import { Disposable } from '../models';
+import { createValueEventEmitter } from '../utils';
+
+type Page = navigationApi.Page;
+
+/** Maps route path patterns to their corresponding Page type. */
+const PAGE_ROUTES: { path: string; page: Page }[] = [
+ { path: RoutePaths.DASHBOARD, page: 'dashboard' },
+ { path: RoutePaths.DASHBOARD_LIST, page: 'dashboard_list' },
+ { path: RoutePaths.QUERY_HISTORY, page: 'query_history' },
+ { path: RoutePaths.SAVED_QUERIES, page: 'saved_queries' },
+ { path: RoutePaths.SQLLAB, page: 'sqllab' },
+ { path: RoutePaths.CHART_ADD, page: 'explore' },
+ { path: RoutePaths.CHART_LIST, page: 'chart_list' },
+ { path: RoutePaths.EXPLORE, page: 'explore' },
+ { path: RoutePaths.EXPLORE_PERMALINK, page: 'explore' },
+ { path: RoutePaths.DATASET_LIST, page: 'dataset_list' },
+ { path: RoutePaths.DATASET_ADD, page: 'dataset' },
+ { path: RoutePaths.DATASET, page: 'dataset' },
+];
+
+function derivePage(pathname: string): Page {
+ for (const { path, page } of PAGE_ROUTES) {
+ if (matchPath(pathname, { path, exact: false })) return page;
+ }
+ return 'home';
+}
+
+const pageEmitter = createValueEventEmitter<Page>(
+ derivePage(window.location.pathname),
Review Comment:
@bito-core-review False positive. Superset is a browser-only SPA with no SSR, and Jest runs under jsdom which provides window.location — so there's no environment where this would throw. The suggested fix (initializing with 'home' and setting the real page later in useNavigationTracker) would actually introduce a subtle regression: getPage() would return 'home' for one render cycle before the hook mounts, potentially causing incorrect initial state in chat or navigation-aware extensions.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3437402058
##########
superset-frontend/src/core/navigation/index.ts:
##########
@@ -0,0 +1,94 @@
+/**
+ * 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.
+ */
+
+/**
+ * @fileoverview Host-internal implementation of the `navigation` namespace.
+ *
+ * Derives the current {@link Page} from the browser location by matching
+ * against {@link RoutePaths}. Call {@link useNavigationTracker} once in the
+ * app shell to keep the page in sync with React Router.
+ */
+
+import { useEffect, useRef } from 'react';
+import { useLocation, matchPath } from 'react-router-dom';
+import type { navigation as navigationApi } from '@apache-superset/core';
+import { RoutePaths } from '../../views/routePaths';
+import { Disposable } from '../models';
+import { createValueEventEmitter } from '../utils';
+
+type Page = navigationApi.Page;
+
+/** Maps route path patterns to their corresponding Page type. */
+const PAGE_ROUTES: { path: string; page: Page }[] = [
+ { path: RoutePaths.DASHBOARD, page: 'dashboard' },
+ { path: RoutePaths.DASHBOARD_LIST, page: 'dashboard_list' },
+ { path: RoutePaths.QUERY_HISTORY, page: 'query_history' },
+ { path: RoutePaths.SAVED_QUERIES, page: 'saved_queries' },
+ { path: RoutePaths.SQLLAB, page: 'sqllab' },
+ { path: RoutePaths.CHART_ADD, page: 'explore' },
+ { path: RoutePaths.CHART_LIST, page: 'chart_list' },
+ { path: RoutePaths.EXPLORE, page: 'explore' },
+ { path: RoutePaths.EXPLORE_PERMALINK, page: 'explore' },
+ { path: RoutePaths.DATASET_LIST, page: 'dataset_list' },
+ { path: RoutePaths.DATASET_ADD, page: 'dataset' },
+ { path: RoutePaths.DATASET, page: 'dataset' },
+];
+
+function derivePage(pathname: string): Page {
+ for (const { path, page } of PAGE_ROUTES) {
+ if (matchPath(pathname, { path, exact: false })) return page;
+ }
+ return 'home';
+}
+
+const pageEmitter = createValueEventEmitter<Page>(
+ derivePage(window.location.pathname),
+);
+
+/** Updates the current page from a pathname. No-op when the page is unchanged. */
+export const notifyLocationChanged = (pathname: string): void => {
+ const next = derivePage(pathname);
+ if (next === pageEmitter.getCurrent()) return;
+ pageEmitter.fire(next);
+};
+
+const getPage: typeof navigationApi.getPage = () => pageEmitter.getCurrent();
+
+const onDidChangePage: typeof navigationApi.onDidChangePage = (
+ listener: (page: Page) => void,
+ thisArgs?: any,
Review Comment:
**Suggestion:** Replace the `any` annotation on `thisArgs` with a concrete or safer type (for example, infer it from `navigationApi.onDidChangePage` or use `unknown`) so the new API wrapper does not introduce an untyped parameter. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The file is a new TypeScript source file, and it introduces `any` in the `thisArgs` parameter. This directly violates the rule against new or modified TypeScript/TSX code using the `any` type.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0a275afc612441419180a2fd9a56b451&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0a275afc612441419180a2fd9a56b451&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/navigation/index.ts
**Line:** 75:75
**Comment:**
*Custom Rule: Replace the `any` annotation on `thisArgs` with a concrete or safer type (for example, infer it from `navigationApi.onDidChangePage` or use `unknown`) so the new API wrapper does not introduce an untyped parameter.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=41ceae4a8a1ee076e50e7a023356750686a8fdcb10d3ad927b9690b5e23ccbbc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=41ceae4a8a1ee076e50e7a023356750686a8fdcb10d3ad927b9690b5e23ccbbc&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3438739146
##########
docs/developer_docs/extensions/extension-points/chat.md:
##########
@@ -0,0 +1,140 @@
+---
+title: Chat
+sidebar_position: 3
+---
+
+<!--
+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.
+-->
+
+# Chat Contributions
+
+Extensions can add a chat interface to Superset by registering a trigger and a panel. The host owns the layout, open/close state, and display mode — the extension only needs to provide the UI components.
+
+## Overview
+
+A chat registration consists of two React components:
+
+| Component | Role |
+|-----------|------|
+| **Trigger** | Always-visible entry point (e.g., a floating button). Rendered in the bottom-right corner in floating mode, or as a fixed overlay in panel mode. |
+| **Panel** | The chat UI itself (message list, input, etc.). Mounted by the host in the active display mode. |
+
+## Display Modes
+
+The host supports two display modes, switchable by the user or the extension at runtime:
+
+| Mode | Behavior |
+|------|----------|
+| `floating` | Panel floats above page content, anchored to the bottom-right corner. |
+| `panel` | Panel is docked to the right side of the application as a resizable sidebar, sitting beside the page content. |
+
+The user's last selected mode and open/closed state are persisted across page reloads.
+
+## Registering a Chat
+
+Call `chat.registerChat` from your extension's entry point with a descriptor, a trigger factory, and a panel factory:
+
+```tsx
+import { chat } from '@apache-superset/core';
+import ChatTrigger from './ChatTrigger';
+import ChatPanel from './ChatPanel';
+
+chat.registerChat(
+ { id: 'my-org.my-chat', name: 'My Chat' },
+ ChatTrigger,
+ ChatPanel,
+);
+```
+
+Only one chat registration is active at a time. If a second extension calls `registerChat`, it replaces the first and a warning is logged.
+
+## Opening and Closing the Chat
+
+The trigger component is responsible for toggling the panel. Use `chat.isOpen()`, `chat.open()`, and `chat.close()` to control visibility:
+
+```tsx
+import { chat } from '@apache-superset/core';
+
+export default function ChatTrigger() {
+ return (
+ <button onClick={() => (chat.isOpen() ? chat.close() : chat.open())}>
+ 💬
+ </button>
+ );
+}
+```
+
+You can also subscribe to open/close events from any component:
+
+```tsx
+useEffect(() => {
+ const { dispose } = chat.onDidOpen(() => console.log('chat opened'));
+ return dispose;
+}, []);
+```
+
+## Changing the Display Mode
+
+Call `chat.setDisplayMode` to switch between `'floating'` and `'panel'` modes. In your panel component, subscribe to `onDidChangeDisplayMode` to react to changes (including those triggered by the user):
+
+```tsx
+import { useState, useEffect } from 'react';
+import { chat } from '@apache-superset/core';
+
+export default function ChatPanel() {
+ const [mode, setMode] = useState(chat.getDisplayMode());
+
+ useEffect(() => {
+ const { dispose } = chat.onDidChangeDisplayMode(m => setMode(m));
+ return dispose;
+ }, []);
+
+ return (
+ <div style={{ height: mode === 'panel' ? '100%' : '80vh' }}>
+ <button onClick={() => chat.setDisplayMode(mode === 'panel' ? 'floating' : 'panel')}>
+ {mode === 'panel' ? 'Float' : 'Dock'}
+ </button>
+ {/* message list and input */}
+ </div>
+ );
+}
+```
+
+## Chat API Reference
+
+All methods are available on the `chat` namespace from `@apache-superset/core`:
+
+| Method / Event | Description |
+|----------------|-------------|
+| `registerChat(descriptor, trigger, panel)` | Register a chat extension. Returns a `Disposable` to unregister. |
+| `open()` | Open the chat panel. No-op if already open or no registration. |
+| `close()` | Close the chat panel. |
+| `isOpen()` | Returns `true` if the panel is currently open. |
+| `getDisplayMode()` | Returns the current display mode (`'floating'` or `'panel'`). |
+| `setDisplayMode(mode)` | Switch between `'floating'` and `'panel'` mode. |
+| `onDidOpen(listener)` | Subscribe to panel open events. Returns a `Disposable`. |
+| `onDidClose(listener)` | Subscribe to panel close events. Returns a `Disposable`. |
+| `onDidChangeDisplayMode(listener)` | Subscribe to display mode changes. Returns a `Disposable`. |
+| `onDidRegisterChat(listener)` | Subscribe to registration events. |
+| `onDidUnregisterChat(listener)` | Subscribe to unregistration events. |
+
Review Comment:
@michael-s-molina would it make sense to add `onDidResizePanel()` here?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3439073021
##########
superset-frontend/src/core/navigation/index.ts:
##########
@@ -0,0 +1,94 @@
+/**
+ * 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.
+ */
+
+/**
+ * @fileoverview Host-internal implementation of the `navigation` namespace.
+ *
+ * Derives the current {@link Page} from the browser location by matching
+ * against {@link RoutePaths}. Call {@link useNavigationTracker} once in the
+ * app shell to keep the page in sync with React Router.
+ */
+
+import { useEffect, useRef } from 'react';
+import { useLocation, matchPath } from 'react-router-dom';
+import type { navigation as navigationApi } from '@apache-superset/core';
+import { RoutePaths } from '../../views/routePaths';
+import { Disposable } from '../models';
+import { createValueEventEmitter } from '../utils';
+
+type Page = navigationApi.Page;
+
+/** Maps route path patterns to their corresponding Page type. */
+const PAGE_ROUTES: { path: string; page: Page }[] = [
+ { path: RoutePaths.DASHBOARD, page: 'dashboard' },
+ { path: RoutePaths.DASHBOARD_LIST, page: 'dashboard_list' },
+ { path: RoutePaths.QUERY_HISTORY, page: 'query_history' },
+ { path: RoutePaths.SAVED_QUERIES, page: 'saved_queries' },
+ { path: RoutePaths.SQLLAB, page: 'sqllab' },
+ { path: RoutePaths.CHART_ADD, page: 'explore' },
+ { path: RoutePaths.CHART_LIST, page: 'chart_list' },
+ { path: RoutePaths.EXPLORE, page: 'explore' },
+ { path: RoutePaths.EXPLORE_PERMALINK, page: 'explore' },
+ { path: RoutePaths.DATASET_LIST, page: 'dataset_list' },
+ { path: RoutePaths.DATASET_ADD, page: 'dataset' },
+ { path: RoutePaths.DATASET, page: 'dataset' },
+];
+
+function derivePage(pathname: string): Page {
+ for (const { path, page } of PAGE_ROUTES) {
+ if (matchPath(pathname, { path, exact: false })) return page;
+ }
+ return 'home';
+}
+
+const pageEmitter = createValueEventEmitter<Page>(
+ derivePage(window.location.pathname),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Browser API at module load</b></div>
<div id="fix">
Module-level code on line 61 calls `window.location.pathname` directly at import time, causing a ReferenceError in non-browser environments (SSR, Jest, unit tests). The `pageEmitter` must be initialized with a static default value (`'home'`), with the actual page set when `useNavigationTracker` runs. (See also: [CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #9a27e6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3437430491
##########
superset-frontend/src/core/chat/ChatProvider.ts:
##########
@@ -0,0 +1,203 @@
+/**
+ * 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 { ComponentType } from 'react';
+import type { chat as chatApi } from '@apache-superset/core';
+import {
+ LocalStorageKeys,
+ getItem,
+ setItem,
+} from 'src/utils/localStorageHelpers';
+import { Disposable } from '../models';
+import { createValueEventEmitter, createEventEmitter } from '../utils';
+
+type Chat = chatApi.Chat;
+type DisplayMode = chatApi.DisplayMode;
+
+/**
+ * Singleton manager for the chat provider.
+ * Handles registration, open/close state, and display mode.
+ */
+class ChatProvider {
+ private static instance: ChatProvider;
+
+ private chat: Chat | undefined;
+
+ private trigger: ComponentType | undefined;
+
+ private panel: ComponentType | undefined;
+
+ private opened: boolean;
+
+ private stateSubscribers = new Set<() => void>();
+
+ private registerEmitter = createEventEmitter<Chat>();
+
+ private unregisterEmitter = createEventEmitter<Chat>();
+
+ private openEmitter = createEventEmitter<void>();
+
+ private closeEmitter = createEventEmitter<void>();
+
+ private resizePanelEmitter = createEventEmitter<{ width: number }>();
+
+ private modeEmitter: ReturnType<typeof createValueEventEmitter<DisplayMode>>;
+
+ private constructor() {
+ const persisted = getItem(LocalStorageKeys.ChatState, {
+ open: false,
+ mode: 'floating',
+ });
+ const mode = (
+ persisted.mode === 'panel' ? 'panel' : 'floating'
+ ) as DisplayMode;
+ this.opened = persisted.open === true;
+ this.modeEmitter = createValueEventEmitter<DisplayMode>(mode);
+ }
+
+ public static getInstance(): ChatProvider {
+ if (!ChatProvider.instance) {
+ ChatProvider.instance = new ChatProvider();
+ }
+ return ChatProvider.instance;
+ }
+
+ public subscribe = (listener: () => void): (() => void) => {
+ this.stateSubscribers.add(listener);
+ return () => this.stateSubscribers.delete(listener);
+ };
+
+ private notifyState(): void {
+ setItem(LocalStorageKeys.ChatState, {
+ open: this.opened,
+ mode: this.modeEmitter.getCurrent(),
+ });
+ this.stateSubscribers.forEach(fn => fn());
+ }
+
+ private closePanel(): void {
+ this.opened = false;
+ this.closeEmitter.fire();
+ }
+
+ public registerChat(
+ chat: Chat,
+ trigger: ComponentType,
+ panel: ComponentType,
+ ): Disposable {
+ if (this.chat) {
+ // eslint-disable-next-line no-console
+ console.warn(
+ `[Superset] Multiple chat extensions registered. Using "${chat.id}"; discarding "${this.chat.id}".`,
+ );
+ if (this.opened) this.closePanel();
+ }
+
+ this.chat = chat;
+ this.trigger = trigger;
+ this.panel = panel;
+ this.registerEmitter.fire(chat);
Review Comment:
**Suggestion:** Replacing an existing chat registration never emits an unregister event for the displaced chat, so listeners subscribed to unregistration events will miss that transition and can keep stale state/resources. Emit `onDidUnregisterChat` for the previous chat before switching to the new one. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Replacing chat never fires unregistration event for previous chat.
- ⚠️ Listeners may leak resources or retain stale chat state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In an extension (or test), subscribe to unregistration events via
`chat.onDidUnregisterChat(listener)` exported from `@apache-superset/core`
(`superset-frontend/packages/superset-core/src/chat/index.ts` lines 33-36), which is wired
to `ChatProvider.onDidUnregisterChat` in `superset-frontend/src/core/chat/index.ts` lines
7-12.
2. Register an initial chat with `chat.registerChat({ id: 'first.chat', name: 'First' },
Trigger1, Panel1)`; this calls `ChatProvider.registerChat` in
`superset-frontend/src/core/chat/ChatProvider.ts` lines 99-117, setting `this.chat` to the
first descriptor and firing `registerEmitter`.
3. Without disposing the first registration's `Disposable`, register another chat with
`chat.registerChat({ id: 'second.chat', name: 'Second' }, Trigger2, Panel2)`; inside
`ChatProvider.registerChat` the `if (this.chat)` block at lines 104-110 logs a warning and
(if open) calls `closePanel()`, then lines 112-115 replace `this.chat`, `this.trigger`,
and `this.panel` with the new registration and fire only `registerEmitter`.
4. Observe that the `listener` passed to `onDidUnregisterChat` is never invoked when the
first chat is displaced, even though `getChat()` (via the same API mapping) now returns
the second descriptor; the prior chat is effectively unregistered without any
corresponding `onDidUnregisterChat` lifecycle event.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4e4b6a768b3940a3abb5d64c5c436130&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4e4b6a768b3940a3abb5d64c5c436130&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/chat/ChatProvider.ts
**Line:** 104:115
**Comment:**
*Api Mismatch: Replacing an existing chat registration never emits an unregister event for the displaced chat, so listeners subscribed to unregistration events will miss that transition and can keep stale state/resources. Emit `onDidUnregisterChat` for the previous chat before switching to the new one.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=9dbafcf4b34113fed907a17fbc780e807afbcec8855adcd3c0cdb38de8b84693&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=9dbafcf4b34113fed907a17fbc780e807afbcec8855adcd3c0cdb38de8b84693&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/chat/ChatProvider.ts:
##########
@@ -0,0 +1,203 @@
+/**
+ * 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 { ComponentType } from 'react';
+import type { chat as chatApi } from '@apache-superset/core';
+import {
+ LocalStorageKeys,
+ getItem,
+ setItem,
+} from 'src/utils/localStorageHelpers';
+import { Disposable } from '../models';
+import { createValueEventEmitter, createEventEmitter } from '../utils';
+
+type Chat = chatApi.Chat;
+type DisplayMode = chatApi.DisplayMode;
+
+/**
+ * Singleton manager for the chat provider.
+ * Handles registration, open/close state, and display mode.
+ */
+class ChatProvider {
+ private static instance: ChatProvider;
+
+ private chat: Chat | undefined;
+
+ private trigger: ComponentType | undefined;
+
+ private panel: ComponentType | undefined;
+
+ private opened: boolean;
+
+ private stateSubscribers = new Set<() => void>();
+
+ private registerEmitter = createEventEmitter<Chat>();
+
+ private unregisterEmitter = createEventEmitter<Chat>();
+
+ private openEmitter = createEventEmitter<void>();
+
+ private closeEmitter = createEventEmitter<void>();
+
+ private resizePanelEmitter = createEventEmitter<{ width: number }>();
+
+ private modeEmitter: ReturnType<typeof createValueEventEmitter<DisplayMode>>;
+
+ private constructor() {
+ const persisted = getItem(LocalStorageKeys.ChatState, {
+ open: false,
+ mode: 'floating',
+ });
+ const mode = (
+ persisted.mode === 'panel' ? 'panel' : 'floating'
+ ) as DisplayMode;
+ this.opened = persisted.open === true;
+ this.modeEmitter = createValueEventEmitter<DisplayMode>(mode);
+ }
+
+ public static getInstance(): ChatProvider {
+ if (!ChatProvider.instance) {
+ ChatProvider.instance = new ChatProvider();
+ }
+ return ChatProvider.instance;
+ }
+
+ public subscribe = (listener: () => void): (() => void) => {
+ this.stateSubscribers.add(listener);
+ return () => this.stateSubscribers.delete(listener);
+ };
+
+ private notifyState(): void {
+ setItem(LocalStorageKeys.ChatState, {
+ open: this.opened,
+ mode: this.modeEmitter.getCurrent(),
+ });
+ this.stateSubscribers.forEach(fn => fn());
+ }
+
+ private closePanel(): void {
+ this.opened = false;
+ this.closeEmitter.fire();
+ }
+
+ public registerChat(
+ chat: Chat,
+ trigger: ComponentType,
+ panel: ComponentType,
+ ): Disposable {
+ if (this.chat) {
+ // eslint-disable-next-line no-console
+ console.warn(
+ `[Superset] Multiple chat extensions registered. Using "${chat.id}"; discarding "${this.chat.id}".`,
+ );
+ if (this.opened) this.closePanel();
+ }
+
+ this.chat = chat;
+ this.trigger = trigger;
+ this.panel = panel;
+ this.registerEmitter.fire(chat);
+ this.notifyState();
+
+ return new Disposable(() => {
+ if (this.chat?.id !== chat.id) return;
+ this.chat = undefined;
+ this.trigger = undefined;
+ this.panel = undefined;
+ this.unregisterEmitter.fire(chat);
+ if (this.opened) this.closePanel();
+ this.notifyState();
+ });
Review Comment:
**Suggestion:** The disposal guard only compares `id`, so if a new registration reuses the same chat id, disposing the old registration will incorrectly unregister the newer active chat. Track registration identity (for example by comparing object reference or a generated token) instead of just `id` before clearing provider state. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Disposing old registration can unregister latest active chat unexpectedly.
- ⚠️ Chat UI becomes unavailable until new registration occurs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In an extension, import the chat API from `@apache-superset/core`
(`superset-frontend/packages/superset-core/src/chat/index.ts`, registerChat declaration
around lines 54-57) and call `chat.registerChat({ id: 'acme.chat', name: 'First' },
Trigger1, Panel1)` to register the first chat; keep the returned `Disposable` as `d1`.
2. Without disposing `d1`, call `chat.registerChat({ id: 'acme.chat', name: 'Second' },
Trigger2, Panel2)` a second time (same `id`, different descriptor/components); this flows
into `ChatProvider.registerChat` in `superset-frontend/src/core/chat/ChatProvider.ts`
lines 99-117 where `this.chat` is updated to the new descriptor and the old one is
effectively displaced.
3. Later, dispose the original registration by calling `d1.dispose()`, which invokes the
`Disposable` callback defined at `ChatProvider.ts` lines 118-126: `if (this.chat?.id !==
chat.id) return;` followed by clearing `this.chat`, `this.trigger`, `this.panel`, firing
`unregisterEmitter`, and closing the panel.
4. Because both registrations used the same `chat.id` string, the guard at line 119
succeeds even though `this.chat` now refers to the newer registration, so the dispose of
`d1` incorrectly unregisters the second (current) chat: `chat.getChat()` (mapped via
`superset-frontend/src/core/chat/index.ts` lines 7-12) now returns `undefined` and any
listeners see an unregistration for the active chat while the second registration's
`Disposable` is still held.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c7c44c209de845aa8f028030eb749a93&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c7c44c209de845aa8f028030eb749a93&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/chat/ChatProvider.ts
**Line:** 118:126
**Comment:**
*Logic Error: The disposal guard only compares `id`, so if a new registration reuses the same chat id, disposing the old registration will incorrectly unregister the newer active chat. Track registration identity (for example by comparing object reference or a generated token) instead of just `id` before clearing provider state.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=c23b9183cdf2ab05aa18e237fffca0ab384cf069dd4b0218382c7c4eedf63d0f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=c23b9183cdf2ab05aa18e237fffca0ab384cf069dd4b0218382c7c4eedf63d0f&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3437435592
##########
superset-frontend/src/core/menus/index.ts:
##########
@@ -117,16 +118,11 @@ export const useMenu = (location: string): Menu | undefined =>
export const onDidRegisterMenuItem: typeof menusApi.onDidRegisterMenuItem = (
listener: (e: MenuItemRegisteredEvent) => void,
-): Disposable => {
- registerListeners.add(listener);
- return new Disposable(() => registerListeners.delete(listener));
-};
+): Disposable => registerEmitter.subscribe(listener);
Review Comment:
**Suggestion:** This wrapper drops the optional `thisArgs` parameter from the `Event` contract, so listeners that rely on context binding will run with the wrong `this` value. Forward `thisArgs` to `registerEmitter.subscribe(...)` to preserve the public API behavior. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Menu registration listeners cannot rely on bound `this` context.
- ⚠️ Extension APIs diverge from documented `Event` type behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the public event type in
`superset-frontend/packages/superset-core/src/common/index.ts:190-24`, where `Event<T>` is
declared as a callable `(listener: (e: T) => any, thisArgs?: any): Disposable`, explicitly
supporting an optional `thisArgs` context parameter.
2. Observe the menus API declaration in
`superset-frontend/packages/superset-core/src/menus/index.ts:133-138`, where
`onDidRegisterMenuItem` is declared as `export declare const onDidRegisterMenuItem:
Event<MenuItemRegisteredEvent>;`, so extension authors are allowed to call
`menus.onDidRegisterMenuItem(listener, thisArgs)`.
3. Inspect the host implementation in `superset-frontend/src/core/menus/index.ts:119-121`,
where `onDidRegisterMenuItem` is implemented as `(listener: (e: MenuItemRegisteredEvent)
=> void): Disposable => registerEmitter.subscribe(listener);` — the wrapper only accepts
`listener` and forwards a single argument to `registerEmitter.subscribe`, silently
dropping any `thisArgs` passed by the caller.
4. Follow the runtime path: when an extension calls `menus.registerMenuItem(...)`
(implementation at `superset-frontend/src/core/menus/index.ts:66-80`), `notifyRegister` at
lines 55-59 fires `registerEmitter.fire(event)`, which in turn invokes all listeners
registered via `onDidRegisterMenuItem`; because `thisArgs` was never forwarded, any
extension code that relied on `this` (e.g., `menus.onDidRegisterMenuItem(function (e) {
this.handleMenu(e); }, someContext)`) will see `this === undefined` instead of
`someContext`, violating the `Event` contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=051a0d23ab034c97b5a93849ab58d1da&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=051a0d23ab034c97b5a93849ab58d1da&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/menus/index.ts
**Line:** 119:121
**Comment:**
*Api Mismatch: This wrapper drops the optional `thisArgs` parameter from the `Event` contract, so listeners that rely on context binding will run with the wrong `this` value. Forward `thisArgs` to `registerEmitter.subscribe(...)` to preserve the public API behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=f898a30599758728c272218f9f673621302c82ac31d06e4e262ed35499e54673&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=f898a30599758728c272218f9f673621302c82ac31d06e4e262ed35499e54673&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/menus/index.ts:
##########
@@ -117,16 +118,11 @@ export const useMenu = (location: string): Menu | undefined =>
export const onDidRegisterMenuItem: typeof menusApi.onDidRegisterMenuItem = (
listener: (e: MenuItemRegisteredEvent) => void,
-): Disposable => {
- registerListeners.add(listener);
- return new Disposable(() => registerListeners.delete(listener));
-};
+): Disposable => registerEmitter.subscribe(listener);
export const onDidUnregisterMenuItem: typeof menusApi.onDidUnregisterMenuItem =
- (listener: (e: MenuItemUnregisteredEvent) => void): Disposable => {
- unregisterListeners.add(listener);
- return new Disposable(() => unregisterListeners.delete(listener));
- };
+ (listener: (e: MenuItemUnregisteredEvent) => void): Disposable =>
+ unregisterEmitter.subscribe(listener);
Review Comment:
**Suggestion:** This unregistration event wrapper also omits the optional `thisArgs` argument, which makes callback context binding inconsistent with the `Event` type contract and with other APIs that pass through context. Accept and forward `thisArgs` here as well. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Menu unregistration listeners lose their intended `this` context.
- ⚠️ Extensions observing menu cleanup behave incorrectly on dispose.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. As in suggestion 1, the shared `Event<T>` type in
`superset-frontend/packages/superset-core/src/common/index.ts:190-24` defines a callable
`(listener: (e: T) => any, thisArgs?: any): Disposable`, and
`superset-frontend/packages/superset-core/src/menus/index.ts:133-138` declares
`onDidUnregisterMenuItem` as `Event<MenuItemUnregisteredEvent>`, so consumers may
legitimately call `menus.onDidUnregisterMenuItem(listener, thisArgs)`.
2. In the host implementation at `superset-frontend/src/core/menus/index.ts:123-125`,
`onDidUnregisterMenuItem` is defined as `(listener: (e: MenuItemUnregisteredEvent) =>
void): Disposable => unregisterEmitter.subscribe(listener);`, again only accepting
`listener` and passing a single argument into `unregisterEmitter.subscribe`, discarding
any supplied `thisArgs`.
3. When a menu item is registered via `registerMenuItem`
(`superset-frontend/src/core/menus/index.ts:66-80`), the returned `Disposable`'s `dispose`
function removes the item and calls `notifyUnregister` at lines 60-63, which fires
`unregisterEmitter.fire(event)` to notify all `onDidUnregisterMenuItem` listeners.
4. If an extension subscribes with context, e.g. `menus.onDidUnregisterMenuItem(function
(e) { this.onMenuRemoved(e); }, myContext)`, the call goes through the wrapper in
`src/core/menus/index.ts:123-125` and into `createEventEmitter` in
`src/core/utils.ts:44-50` without the `thisArgs` argument; as a result, the listener is
stored unbound and will be invoked with `this === undefined` when a menu is unregistered,
breaking the expected `Event` behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f46056837e284c839d487bafaf2a9687&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f46056837e284c839d487bafaf2a9687&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/core/menus/index.ts
**Line:** 123:125
**Comment:**
*Api Mismatch: This unregistration event wrapper also omits the optional `thisArgs` argument, which makes callback context binding inconsistent with the `Event` type contract and with other APIs that pass through context. Accept and forward `thisArgs` here as well.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=87e360d7c8eb1ddbacfeeaabc25689d761bbd345651623ee2003c4e1025183dd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41205&comment_hash=87e360d7c8eb1ddbacfeeaabc25689d761bbd345651623ee2003c4e1025183dd&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41205:
URL: https://github.com/apache/superset/pull/41205#issuecomment-4744040971
<!-- Bito Reply -->
The issue is valid. The `any` type in `ComponentType<any>` and `ComponentProps<any>` in `superset-frontend/src/views/routes.tsx` bypasses TypeScript's type checking, which is discouraged.
To resolve this, you can replace `any` with `unknown` or a more specific type if available. Given the context, `unknown` is a safer alternative that forces explicit type narrowing when the component or props are used.
Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?
**superset-frontend/src/views/routes.tsx**
```
type Routes = {
path: string;
Component: ComponentType<unknown>;
Fallback?: ComponentType<unknown>;
props?: ComponentProps<unknown>;
}[];
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(extensions): add Chat contribution type (SIP-214) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #41205:
URL: https://github.com/apache/superset/pull/41205#discussion_r3444190285
##########
superset-frontend/src/views/App.tsx:
##########
@@ -79,42 +84,137 @@ const LocationPathnameLogger = () => {
return <></>;
};
+const CHAT_PANEL_DEFAULT_WIDTH = 400;
+const CHAT_PANEL_MIN_WIDTH = 280;
+
+const RouteSwitch = () => (
+ <Switch>
+ {routes.map(({ path, Component, props = {}, Fallback = Loading }) => (
+ <Route path={path} key={path}>
+ <Suspense fallback={<Fallback />}>
+ <ErrorBoundary
+ css={css`
+ margin: 16px;
Review Comment:
We can use `theme.sizeUnit`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org