You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "EnxDev (via GitHub)" <gi...@apache.org> on 2026/06/12 19:19:46 UTC
[PR] feat(extensions): add dedicated chat contribution type (#41000) [superset]
EnxDev opened a new pull request, #41006:
URL: https://github.com/apache/superset/pull/41006
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] 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): Introducing a Chat extension in Superset [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41006:
URL: https://github.com/apache/superset/pull/41006#issuecomment-4718774407
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41006?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.58%. Comparing base ([`6d08e79`](https://app.codecov.io/gh/apache/superset/commit/6d08e79259a327fa60b7a6d7e9a7808de2afb839?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ec623f3`](https://app.codecov.io/gh/apache/superset/commit/ec623f3b9310b5cc1ab28ff29a14db76d8101b72?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41006 +/- ##
==========================================
- Coverage 55.61% 55.58% -0.04%
==========================================
Files 2652 2652
Lines 144790 144789 -1
Branches 33410 33409 -1
==========================================
- Hits 80530 80485 -45
- Misses 63533 63577 +44
Partials 727 727
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41006/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/41006/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/41006/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.90% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/41006/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.90% <ø> (-0.07%)` | :arrow_down: |
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/41006?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 dedicated chat contribution type (#41000) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #41006:
URL: https://github.com/apache/superset/pull/41006#discussion_r3406020036
##########
superset-frontend/packages/superset-core/src/dashboard/index.ts:
##########
@@ -0,0 +1,114 @@
+/**
+ * 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 Dashboard namespace for Superset extensions (P3).
+ *
+ * Exposes dashboard identity and filter state as a stable semantic API.
+ * Extensions must not depend on the Redux dashboard slice structure directly.
+ */
+
+import { Event } from '../common';
Review Comment:
I think it's better to remove the additional namespaces from the scope of this first phase as we'll need to discuss each one individually. Let's focus on the `chat` and `navigation` namespaces and introduce `dashboard`, `dataset`, `explore` in follow-up PRs.
--
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 dedicated chat contribution type (#41000) [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 #41006:
URL: https://github.com/apache/superset/pull/41006#discussion_r3405722865
##########
superset-frontend/src/core/dashboard/index.test.ts:
##########
@@ -0,0 +1,220 @@
+/**
+ * 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.
+ */
+
+// ---------------------------------------------------------------------------
+// Captured listeners — allows tests to trigger action notifications manually.
+// ---------------------------------------------------------------------------
+type ListenerEntry = {
+ predicate: (action: { type: string }) => boolean;
+ effect: (action: { type: string }) => void;
+};
+
+const capturedListeners: ListenerEntry[] = [];
+
+// Declared before jest.mock so the factory closure can reference it.
+let mockState: Record<string, unknown>;
+
+jest.mock('src/views/store', () => ({
+ store: { getState: () => mockState, dispatch: jest.fn() },
+ listenerMiddleware: {
+ startListening: (opts: {
+ predicate: (action: { type: string }) => boolean;
+ effect: (action: { type: string }) => void;
+ }) => {
+ const entry = { predicate: opts.predicate, effect: opts.effect };
+ capturedListeners.push(entry);
+ return () => {
+ const idx = capturedListeners.indexOf(entry);
+ if (idx !== -1) capturedListeners.splice(idx, 1);
+ };
+ },
+ },
+}));
+
+jest.mock('../navigation', () => ({
+ navigation: { getPageType: jest.fn(() => 'dashboard') },
+}));
+
+function dispatch(actionType: string) {
+ const action = { type: actionType };
+ capturedListeners
+ .filter(e => e.predicate(action))
+ .forEach(e => e.effect(action));
+}
+
+// Imported after mocks
+// eslint-disable-next-line import/first
+import { dashboard } from './index';
+
+function makeState(
+ overrides: Partial<{
+ dashboardInfo: unknown;
+ nativeFilters: unknown;
+ dataMask: unknown;
+ sliceEntities: unknown;
+ dashboardLayout: unknown;
+ }> = {},
+) {
+ return {
+ dashboardInfo: { id: 1, dashboard_title: 'Sales', slug: 'sales' },
+ nativeFilters: { filters: { 'filter-1': { name: 'Region' } } },
+ dataMask: { 'filter-1': { filterState: { value: ['West'] } } },
+ sliceEntities: { slices: {} },
+ dashboardLayout: { present: {} },
+ ...overrides,
+ };
+}
+
+beforeEach(() => {
+ mockState = makeState();
+});
+
+afterEach(() => {
+ capturedListeners.length = 0;
+ jest.restoreAllMocks();
+});
+
+test('getCurrentDashboard returns undefined when not on dashboard page', () => {
+ const { navigation } = jest.requireMock('../navigation');
+ (navigation.getPageType as jest.Mock).mockReturnValueOnce('explore');
+ expect(dashboard.getCurrentDashboard()).toBeUndefined();
+});
+
+test('getCurrentDashboard returns undefined when dashboardInfo is absent', () => {
+ mockState = makeState({ dashboardInfo: undefined });
+ expect(dashboard.getCurrentDashboard()).toBeUndefined();
+});
+
+test('getCurrentDashboard returns dashboard context with active filters', () => {
+ expect(dashboard.getCurrentDashboard()).toEqual({
+ dashboardId: 1,
+ title: 'Sales',
+ filters: [{ filterId: 'filter-1', label: 'Region', value: ['West'] }],
+ // No charts on the (empty) layout fixture.
+ charts: [],
+ });
+});
+
+test('getCurrentDashboard reports charts placed on the dashboard layout', () => {
+ mockState = makeState({
+ sliceEntities: {
+ slices: {
+ 42: {
+ slice_name: 'Revenue by Region',
+ viz_type: 'echarts_timeseries_bar',
+ datasource_id: 7,
+ datasource_name: 'cleaned_sales',
+ },
+ },
+ },
+ dashboardLayout: {
+ present: {
+ 'CHART-abc': { id: 'CHART-abc', type: 'CHART', meta: { chartId: 42 } },
+ // A chart id with no matching slice entity still appears, with blanks.
+ 'CHART-def': { id: 'CHART-def', type: 'CHART', meta: { chartId: 99 } },
+ // Non-chart components are ignored.
+ 'TAB-xyz': { id: 'TAB-xyz', type: 'TAB', meta: {} },
+ },
+ },
+ });
+
+ expect(dashboard.getCurrentDashboard()?.charts).toEqual([
+ {
+ chartId: 42,
+ chartName: 'Revenue by Region',
+ vizType: 'echarts_timeseries_bar',
+ datasourceId: 7,
+ datasourceName: 'cleaned_sales',
+ isVisible: true,
+ },
+ {
+ chartId: 99,
+ chartName: '',
+ vizType: '',
+ datasourceId: null,
+ datasourceName: null,
+ isVisible: true,
+ },
+ ]);
+});
+
+test('getCurrentDashboard excludes filters with null value', () => {
+ mockState = makeState({
+ dataMask: { 'filter-1': { filterState: { value: null } } },
+ });
+ expect(dashboard.getCurrentDashboard()?.filters).toHaveLength(0);
+});
+
+test('getCurrentDashboard excludes dataMask entries not in nativeFilters', () => {
+ mockState = makeState({
+ dataMask: { 'chart-filter': { filterState: { value: 'foo' } } },
+ });
+ expect(dashboard.getCurrentDashboard()?.filters).toHaveLength(0);
+});
+
+test('filter array value is a defensive copy — mutation does not affect Redux state', () => {
+ const ctx = dashboard.getCurrentDashboard();
+ const original = [
+ ...(mockState as any).dataMask['filter-1'].filterState.value,
Review Comment:
**Suggestion:** Replace the `any` cast with a concrete state shape for the mocked Redux data (or a narrowly scoped interface) so the test accesses typed properties without using `any`. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The final file still contains a TypeScript `any` cast in newly added test code.
This directly violates the rule that modified TypeScript/TSX code should not use `any` when a concrete or narrower type can be used.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fb8201d64adb458096956ee0f40f6d09&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fb8201d64adb458096956ee0f40f6d09&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/dashboard/index.test.ts
**Line:** 174:174
**Comment:**
*Custom Rule: Replace the `any` cast with a concrete state shape for the mocked Redux data (or a narrowly scoped interface) so the test accesses typed properties without using `any`.
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%2F41006&comment_hash=86539dd5b204af5195d85b4cc52b191fc6f0ffd77b2c76d96842974c2611af22&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41006&comment_hash=86539dd5b204af5195d85b4cc52b191fc6f0ffd77b2c76d96842974c2611af22&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/dashboard/index.test.ts:
##########
@@ -0,0 +1,220 @@
+/**
+ * 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.
+ */
+
+// ---------------------------------------------------------------------------
+// Captured listeners — allows tests to trigger action notifications manually.
+// ---------------------------------------------------------------------------
+type ListenerEntry = {
+ predicate: (action: { type: string }) => boolean;
+ effect: (action: { type: string }) => void;
+};
+
+const capturedListeners: ListenerEntry[] = [];
+
+// Declared before jest.mock so the factory closure can reference it.
+let mockState: Record<string, unknown>;
+
+jest.mock('src/views/store', () => ({
+ store: { getState: () => mockState, dispatch: jest.fn() },
+ listenerMiddleware: {
+ startListening: (opts: {
+ predicate: (action: { type: string }) => boolean;
+ effect: (action: { type: string }) => void;
+ }) => {
+ const entry = { predicate: opts.predicate, effect: opts.effect };
+ capturedListeners.push(entry);
+ return () => {
+ const idx = capturedListeners.indexOf(entry);
+ if (idx !== -1) capturedListeners.splice(idx, 1);
+ };
+ },
+ },
+}));
+
+jest.mock('../navigation', () => ({
+ navigation: { getPageType: jest.fn(() => 'dashboard') },
+}));
+
+function dispatch(actionType: string) {
+ const action = { type: actionType };
+ capturedListeners
+ .filter(e => e.predicate(action))
+ .forEach(e => e.effect(action));
+}
+
+// Imported after mocks
+// eslint-disable-next-line import/first
+import { dashboard } from './index';
+
+function makeState(
+ overrides: Partial<{
+ dashboardInfo: unknown;
+ nativeFilters: unknown;
+ dataMask: unknown;
+ sliceEntities: unknown;
+ dashboardLayout: unknown;
+ }> = {},
+) {
+ return {
+ dashboardInfo: { id: 1, dashboard_title: 'Sales', slug: 'sales' },
+ nativeFilters: { filters: { 'filter-1': { name: 'Region' } } },
+ dataMask: { 'filter-1': { filterState: { value: ['West'] } } },
+ sliceEntities: { slices: {} },
+ dashboardLayout: { present: {} },
+ ...overrides,
+ };
+}
+
+beforeEach(() => {
+ mockState = makeState();
+});
+
+afterEach(() => {
+ capturedListeners.length = 0;
+ jest.restoreAllMocks();
+});
+
+test('getCurrentDashboard returns undefined when not on dashboard page', () => {
+ const { navigation } = jest.requireMock('../navigation');
+ (navigation.getPageType as jest.Mock).mockReturnValueOnce('explore');
+ expect(dashboard.getCurrentDashboard()).toBeUndefined();
+});
+
+test('getCurrentDashboard returns undefined when dashboardInfo is absent', () => {
+ mockState = makeState({ dashboardInfo: undefined });
+ expect(dashboard.getCurrentDashboard()).toBeUndefined();
+});
+
+test('getCurrentDashboard returns dashboard context with active filters', () => {
+ expect(dashboard.getCurrentDashboard()).toEqual({
+ dashboardId: 1,
+ title: 'Sales',
+ filters: [{ filterId: 'filter-1', label: 'Region', value: ['West'] }],
+ // No charts on the (empty) layout fixture.
+ charts: [],
+ });
+});
+
+test('getCurrentDashboard reports charts placed on the dashboard layout', () => {
+ mockState = makeState({
+ sliceEntities: {
+ slices: {
+ 42: {
+ slice_name: 'Revenue by Region',
+ viz_type: 'echarts_timeseries_bar',
+ datasource_id: 7,
+ datasource_name: 'cleaned_sales',
+ },
+ },
+ },
+ dashboardLayout: {
+ present: {
+ 'CHART-abc': { id: 'CHART-abc', type: 'CHART', meta: { chartId: 42 } },
+ // A chart id with no matching slice entity still appears, with blanks.
+ 'CHART-def': { id: 'CHART-def', type: 'CHART', meta: { chartId: 99 } },
+ // Non-chart components are ignored.
+ 'TAB-xyz': { id: 'TAB-xyz', type: 'TAB', meta: {} },
+ },
+ },
+ });
+
+ expect(dashboard.getCurrentDashboard()?.charts).toEqual([
+ {
+ chartId: 42,
+ chartName: 'Revenue by Region',
+ vizType: 'echarts_timeseries_bar',
+ datasourceId: 7,
+ datasourceName: 'cleaned_sales',
+ isVisible: true,
+ },
+ {
+ chartId: 99,
+ chartName: '',
+ vizType: '',
+ datasourceId: null,
+ datasourceName: null,
+ isVisible: true,
+ },
+ ]);
+});
+
+test('getCurrentDashboard excludes filters with null value', () => {
+ mockState = makeState({
+ dataMask: { 'filter-1': { filterState: { value: null } } },
+ });
+ expect(dashboard.getCurrentDashboard()?.filters).toHaveLength(0);
+});
+
+test('getCurrentDashboard excludes dataMask entries not in nativeFilters', () => {
+ mockState = makeState({
+ dataMask: { 'chart-filter': { filterState: { value: 'foo' } } },
+ });
+ expect(dashboard.getCurrentDashboard()?.filters).toHaveLength(0);
+});
+
+test('filter array value is a defensive copy — mutation does not affect Redux state', () => {
+ const ctx = dashboard.getCurrentDashboard();
+ const original = [
+ ...(mockState as any).dataMask['filter-1'].filterState.value,
+ ];
+ (ctx!.filters[0].value as string[]).push('East');
+ expect((mockState as any).dataMask['filter-1'].filterState.value).toEqual(
Review Comment:
**Suggestion:** Remove the `any` cast in this assertion by reusing the same concrete mocked state type used elsewhere in the test, keeping property access type-safe. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The file uses `as any` in the assertion path, which is a direct instance of the banned `any` type in modified TypeScript test code.
This is a real violation of the custom rule.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=abfd515f0a1044efaaf898a9b829d0f2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=abfd515f0a1044efaaf898a9b829d0f2&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/dashboard/index.test.ts
**Line:** 177:177
**Comment:**
*Custom Rule: Remove the `any` cast in this assertion by reusing the same concrete mocked state type used elsewhere in the test, keeping property access type-safe.
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%2F41006&comment_hash=63d9416a9f844007b13fa58fe0ec3ae2b456d4fc4fe10b6562119521cdf740f7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41006&comment_hash=63d9416a9f844007b13fa58fe0ec3ae2b456d4fc4fe10b6562119521cdf740f7&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/dashboard/index.ts:
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+
+/**
+ * Host-internal implementation of the `dashboard` namespace.
+ *
+ * Wraps Redux dashboardInfo and dataMask state and normalizes them into the
+ * stable `DashboardContext` contract. Extensions must not depend on the Redux
+ * slice structure directly.
+ */
+
+import type { dashboard as dashboardApi } from '@apache-superset/core';
+import type { DataMaskStateWithId } from '@superset-ui/core';
+import { HYDRATE_DASHBOARD } from 'src/dashboard/actions/hydrate';
+import {
+ UPDATE_DATA_MASK,
+ SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE,
+} from 'src/dataMask/actions';
+import { store, RootState } from 'src/views/store';
+import { AnyListenerPredicate } from '@reduxjs/toolkit';
+import getChartIdsFromLayout from 'src/dashboard/util/getChartIdsFromLayout';
+import { createActionListener } from '../utils';
+import { navigation } from '../navigation';
+
+type DashboardContext = dashboardApi.DashboardContext;
+type FilterValue = dashboardApi.FilterValue;
+type ChartSummary = NonNullable<DashboardContext['charts']>[number];
+
+function buildChartSummaries(state: RootState): ChartSummary[] {
+ const slices = state.sliceEntities?.slices ?? {};
+ const layout = state.dashboardLayout?.present ?? {};
+
+ // Only charts actually placed on the dashboard layout — `slices` can also
+ // hold entities that are not on the current dashboard.
+ return getChartIdsFromLayout(layout).map(chartId => {
+ const slice = slices[chartId];
+ return {
+ chartId,
+ chartName: slice?.slice_name ?? '',
+ vizType: slice?.viz_type ?? '',
+ datasourceId: slice?.datasource_id ?? null,
+ datasourceName: slice?.datasource_name ?? null,
+ // Tab-accurate visibility is a deferred phase; every chart on the
+ // dashboard is reported visible for now.
+ isVisible: true,
+ };
+ });
+}
+
+function buildDashboardContext(): DashboardContext | undefined {
+ if (navigation.getPageType() !== 'dashboard') return undefined;
+ // `store.getState()` is already typed as RootState, so the slices below are
+ // read with their real types — the host owns this normalization and must
+ // stay type-safe against slice reshapes.
+ const state = store.getState();
+ const info = state.dashboardInfo;
+ if (!info?.id) return undefined;
+
+ const nativeFilters = state.nativeFilters?.filters ?? {};
+ const dataMask: DataMaskStateWithId = state.dataMask ?? {};
+
+ const filters: FilterValue[] = Object.entries(dataMask)
+ .filter(([id, mask]) => {
+ if (!(id in nativeFilters)) return false;
+ const value = mask?.filterState?.value;
+ return value !== null && value !== undefined;
+ })
+ .map(([id, mask]) => {
+ const raw = mask.filterState?.value;
+ return {
+ filterId: id,
+ label: nativeFilters[id]?.name ?? id,
+ value: Array.isArray(raw) ? [...raw] : raw,
+ };
+ });
+
+ return {
+ dashboardId: info.id,
+ title: info.dashboard_title ?? info.slug ?? String(info.id),
+ filters,
+ charts: buildChartSummaries(state),
+ };
+}
+
+const dashboardChangePredicate: AnyListenerPredicate<RootState> = action =>
+ action.type === HYDRATE_DASHBOARD ||
+ action.type === UPDATE_DATA_MASK ||
+ action.type === SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE;
+
+const getCurrentDashboard: typeof dashboardApi.getCurrentDashboard = () =>
+ buildDashboardContext();
+
+const onDidChangeDashboard: typeof dashboardApi.onDidChangeDashboard = (
+ listener: (ctx: DashboardContext) => void,
+ thisArgs?: any,
Review Comment:
**Suggestion:** Replace the `any` annotation on `thisArgs` with a concrete type from the callback context (or a narrow generic/`unknown` if truly unconstrained) to preserve type safety. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new TypeScript file introduces `thisArgs?: any`, which directly violates the rule against using `any` in new or modified TypeScript/TSX code. A concrete type, reusable existing type, or `unknown`/narrower generic should be used instead.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ba476a532d9e436c9d79db68569e4b21&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ba476a532d9e436c9d79db68569e4b21&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/dashboard/index.ts
**Line:** 111:111
**Comment:**
*Custom Rule: Replace the `any` annotation on `thisArgs` with a concrete type from the callback context (or a narrow generic/`unknown` if truly unconstrained) to preserve type safety.
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%2F41006&comment_hash=2683271b1c7c6aa8cccc948345ba2e3f04bf15fcdf0373f8287b97d3e1697057&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41006&comment_hash=2683271b1c7c6aa8cccc948345ba2e3f04bf15fcdf0373f8287b97d3e1697057&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/explore/index.ts:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.
+ */
+
+/**
+ * Host-internal implementation of the `explore` namespace.
+ *
+ * Wraps Redux explore state and normalizes it into the stable `ChartContext`
+ * contract. Extensions must not depend on the Redux slice structure directly.
+ */
+
+import type { explore as exploreApi } from '@apache-superset/core';
+import { HYDRATE_EXPLORE } from 'src/explore/actions/hydrateExplore';
+import {
+ CREATE_NEW_SLICE,
+ SET_FORM_DATA,
+ SLICE_UPDATED,
+ UPDATE_CHART_TITLE,
+} from 'src/explore/actions/exploreActions';
+import { SET_DATASOURCE } from 'src/explore/actions/datasourcesActions';
+import { store, RootState } from 'src/views/store';
+import { AnyListenerPredicate } from '@reduxjs/toolkit';
+import { createActionListener } from '../utils';
+import { navigation } from '../navigation';
+
+type ChartContext = exploreApi.ChartContext;
+
+function buildChartContext(): ChartContext | undefined {
+ if (navigation.getPageType() !== 'explore') return undefined;
+ // `store.getState()` is already RootState; read the typed `explore` slice
+ // directly rather than casting it away.
+ const state = store.getState();
+ const exploreState = state.explore;
+ if (!exploreState) return undefined;
+
+ const { slice, datasource, controls } = exploreState;
+ const vizType: string =
+ (controls?.viz_type?.value as string) ??
+ exploreState.form_data?.viz_type ??
+ '';
+
+ return {
+ chartId: slice?.slice_id ?? null,
+ chartName: exploreState.sliceName ?? slice?.slice_name ?? null,
+ vizType,
+ datasourceId: datasource?.id ?? null,
+ datasourceName:
+ datasource?.table_name ?? datasource?.datasource_name ?? null,
+ };
+}
+
+const exploreChangePredicate: AnyListenerPredicate<RootState> = action =>
+ action.type === HYDRATE_EXPLORE ||
+ action.type === SET_FORM_DATA ||
+ action.type === UPDATE_CHART_TITLE ||
+ action.type === SET_DATASOURCE ||
+ action.type === CREATE_NEW_SLICE ||
+ action.type === SLICE_UPDATED;
+
+const getCurrentChart: typeof exploreApi.getCurrentChart = () =>
+ buildChartContext();
+
+const onDidChangeChart: typeof exploreApi.onDidChangeChart = (
+ listener: (ctx: ChartContext) => void,
+ thisArgs?: any,
Review Comment:
**Suggestion:** Replace the `any` type on `thisArgs` with a safer type (for example `unknown` or the existing event API argument type) so the new code does not introduce an untyped escape hatch. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The final file still introduces a TypeScript `any` usage on the new `thisArgs` parameter. This directly violates the rule against new or modified TypeScript/TSX code using `any`, and a narrower or existing type could be used instead.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=08091a42dfa449f7846bd2b35693afc4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=08091a42dfa449f7846bd2b35693afc4&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/explore/index.ts
**Line:** 80:80
**Comment:**
*Custom Rule: Replace the `any` type on `thisArgs` with a safer type (for example `unknown` or the existing event API argument type) so the new code does not introduce an untyped escape hatch.
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%2F41006&comment_hash=243c977917f4b1a9da80b657621b6330eb28f6025a085f2077839d010d1a8b6a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41006&comment_hash=243c977917f4b1a9da80b657621b6330eb28f6025a085f2077839d010d1a8b6a&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 dedicated chat contribution type (#41000) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #41006:
URL: https://github.com/apache/superset/pull/41006#discussion_r3406008592
##########
superset-frontend/packages/superset-core/src/navigation/index.ts:
##########
@@ -0,0 +1,84 @@
+/**
+ * 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 Navigation namespace for Superset extensions (P3).
+ *
+ * Exposes the current application surface so extensions can react to route
+ * changes without polling. Entity-level context (chart, dashboard, dataset)
+ * is intentionally not included here — use the surface-specific namespace
+ * (`explore`, `dashboard`, `dataset`) to retrieve entity payloads.
+ */
+
+import { Event } from '../common';
+
+/**
+ * The set of top-level application surfaces.
+ *
+ * `'explore'`, `'dashboard'` and `'dataset'` are the single-entity
+ * editing/viewing surfaces where `explore.getCurrentChart()` /
+ * `dashboard.getCurrentDashboard()` / `dataset.getCurrentDataset()` resolve to a
+ * concrete entity. `'chart_list'`, `'dashboard_list'` and `'dataset_list'` are
+ * the browse/list surfaces, distinct from those because no single entity is
+ * active. `'sqllab'` is the SQL editor where `sqlLab.getCurrentTab()` resolves;
+ * `'query_history'` and `'saved_queries'` are the related SQL Lab browse pages,
+ * which are not the editor. `'other'` covers any route not explicitly enumerated.
+ */
+export type PageType =
Review Comment:
Maybe use Page?
--
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): Introducing a Chat extension in Superset [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev closed pull request #41006: feat(extensions): Introducing a Chat extension in Superset
URL: https://github.com/apache/superset/pull/41006
--
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 dedicated chat contribution type (#41000) [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 #41006:
URL: https://github.com/apache/superset/pull/41006#discussion_r3405752699
##########
superset-frontend/src/core/dataset/index.ts:
##########
@@ -0,0 +1,63 @@
+/**
+ * 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.
+ */
+
+/**
+ * Host-internal implementation of the `dataset` namespace.
+ *
+ * Dataset page components call `setCurrentDataset` to publish context as they
+ * load. Extensions consume the stable `DatasetContext` contract; they are
+ * isolated from the page's internal data-fetching implementation.
+ */
+
+import type { dataset as datasetApi } from '@apache-superset/core';
+import { createEmitter } from '../utils';
+
+type DatasetContext = datasetApi.DatasetContext;
+
+const emitter = createEmitter<DatasetContext | undefined>(undefined);
+
+/**
+ * Host-internal: called by the Dataset page when its entity loads or changes.
+ * Not part of the public `@apache-superset/core` API.
+ */
+export const setCurrentDataset = (ctx: DatasetContext | undefined): void => {
+ emitter.fire(ctx);
+};
+
+const getCurrentDataset: typeof datasetApi.getCurrentDataset = () => {
+ const current = emitter.getCurrent();
+ return current ? { ...current } : undefined;
+};
+
+const onDidChangeDataset: typeof datasetApi.onDidChangeDataset = (
+ listener: (ctx: DatasetContext) => void,
+ thisArgs?: unknown,
+) => {
+ const bound = thisArgs ? listener.bind(thisArgs) : listener;
+ // The public contract only emits a concrete context; skip `undefined` clears
+ // so subscribers are never handed an empty value.
+ return emitter.event(ctx => {
+ if (ctx) bound(ctx);
+ });
Review Comment:
**Suggestion:** `onDidChangeDataset` forwards the exact emitter object reference to listeners, so any listener that mutates the received context can corrupt the shared in-memory current dataset and affect other listeners and future `getCurrentDataset()` reads. Pass a cloned context to listeners (for example a shallow copy) so extension callbacks cannot mutate host-owned state. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ dataset.getCurrentDataset returns context mutated by extensions.
- ⚠️ One extension can corrupt dataset metadata for others.
- ⚠️ Debugging dataset-aware extensions becomes significantly harder.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In an extension using `@apache-superset/core`, register a dataset listener as shown in
`packages/superset-core/src/dataset/index.ts:26-34` by calling
`dataset.onDidChangeDataset(ds => { ... })`.
2. Implement the listener to mutate the received context, for example:
`dataset.onDidChangeDataset(ds => { (ds as any).datasetName = 'mutated-name'; });`,
matching the `DatasetContext` shape defined in
`packages/superset-core/src/dataset/index.ts:1-6`.
3. In the host app, when a Dataset page loads, call `setCurrentDataset(ctx)`
(host-internal API defined in `superset-frontend/src/core/dataset/index.ts:39-41`), which
forwards `ctx` into `emitter.fire(ctx)` using the `Emitter<T>` implementation from
`superset-frontend/src/core/utils.ts:83-93`.
4. The `Emitter` stores this exact object reference as `current` (`utils.ts:85-91`) and
calls all listeners; because `onDidChangeDataset` just wraps `emitter.event(ctx => { if
(ctx) bound(ctx); });` (`dataset/index.ts:48-57`), each subscriber receives the same
mutable object reference and any mutation in one listener alters `current`, so subsequent
`dataset.getCurrentDataset()` calls (`dataset/index.ts:43-45`) and other listeners observe
the mutated DatasetContext.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d5dd9bae75134066b2060dc16c17e24c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d5dd9bae75134066b2060dc16c17e24c&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/dataset/index.ts
**Line:** 55:57
**Comment:**
*Stale Reference: `onDidChangeDataset` forwards the exact emitter object reference to listeners, so any listener that mutates the received context can corrupt the shared in-memory current dataset and affect other listeners and future `getCurrentDataset()` reads. Pass a cloned context to listeners (for example a shallow copy) so extension callbacks cannot mutate host-owned 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%2F41006&comment_hash=1f8e709d555bbd003831a4e4936106c910e60ecb847e20f282e09e81c20ad718&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41006&comment_hash=1f8e709d555bbd003831a4e4936106c910e60ecb847e20f282e09e81c20ad718&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 dedicated chat contribution type (#41000) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41006:
URL: https://github.com/apache/superset/pull/41006#issuecomment-4694521261
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 568337f37058dcee38bcf20a0864ee9f8ada0fd6 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a2c5bd2ba33ae0008212cff |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41006--superset-docs-preview.netlify.app](https://deploy-preview-41006--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#q1YqKMrPLShRslJS0lFKKkrMS84AslPzKlJSy_STMxJLdIEKSvJLKgtSgQpSUgty8is9U4BKzBKNkk2TUoySEo2NE1MNDAwsjAyNktPSlGoB) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41006--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 dedicated chat contribution type (#41000) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41006:
URL: https://github.com/apache/superset/pull/41006#issuecomment-4694503760
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): Introducing a Chat extension in Superset [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #41006:
URL: https://github.com/apache/superset/pull/41006#issuecomment-4744215389
Closed in favor of #41205, which includes all the changes from this 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 dedicated chat contribution type (#41000) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #41006:
URL: https://github.com/apache/superset/pull/41006#discussion_r3406020036
##########
superset-frontend/packages/superset-core/src/dashboard/index.ts:
##########
@@ -0,0 +1,114 @@
+/**
+ * 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 Dashboard namespace for Superset extensions (P3).
+ *
+ * Exposes dashboard identity and filter state as a stable semantic API.
+ * Extensions must not depend on the Redux dashboard slice structure directly.
+ */
+
+import { Event } from '../common';
Review Comment:
I think it's better to remove the additional namespaces from the scope of this first phase as we'll need to discuss each one individually. Let's focus on the `chat` and `navigation` namespaces and introduce dashboard, dataset, explore in follow-up PRs.
--
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 dedicated chat contribution type (#41000) [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 #41006:
URL: https://github.com/apache/superset/pull/41006#discussion_r3405745519
##########
superset-frontend/src/core/chat/index.ts:
##########
@@ -0,0 +1,238 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * @fileoverview Host implementation of the `chat` contribution type.
+ *
+ * Chat is a dedicated contribution type, not a view: extensions register via
+ * the public `chat.registerChat()` and the host owns mounting, open/close
+ * state, and the display mode. Multiple chat extensions may register, but the
+ * host applies singleton resolution — the most-recently-registered chat is
+ * active; disposing it falls back to the previous one.
+ *
+ * Open-state policy across active-chat transitions: when the active chat's
+ * identity changes — a takeover by a different id, disposal falling back to a
+ * different id, or disposal of the last chat — the panel is closed (firing
+ * `onDidClose`) so the incoming chat never mounts into an open state it did
+ * not request. A same-id re-registration is an upgrade in place and keeps the
+ * open state.
+ *
+ * The public namespace (`chat`) is exposed to extensions on
+ * `window.superset`; the other exports are host-internal accessors for
+ * ChatMount and are NOT part of the public `@apache-superset/core` API.
+ */
+
+import { ReactElement } from 'react';
+import type { chat as chatApi } from '@apache-superset/core';
+import { Disposable } from '../models';
+import { createEmitter, createEventEmitter } from '../utils';
+
+type Chat = chatApi.Chat;
+type ChatMode = chatApi.ChatMode;
+
+/** A registered chat: its descriptor plus the host-mountable providers. */
+export interface RegisteredChat {
+ /** The chat descriptor passed to `registerChat`. */
+ chat: Chat;
+ /** Renders the collapsed bubble. Hidden by the host in panel mode. */
+ trigger: () => ReactElement;
+ /** Renders the chat panel, mounted per the current {@link ChatMode}. */
+ panel: () => ReactElement;
+ /**
+ * Unique per registration (a same-id re-registration gets a new one). The
+ * host UI keys mounts and fault containment on it, so a replacement resets
+ * crashed error boundaries instead of inheriting their latched state.
+ */
+ registrationId: number;
+}
+
+/**
+ * Immutable snapshot of the whole chat state, rebuilt on every change.
+ * Returned by reference from `getChatSnapshot` so `useSyncExternalStore`
+ * consumers read registrations, open state, and mode from one consistent
+ * object instead of tearing across separate live reads.
+ */
+export interface ChatSnapshot {
+ /** Monotonic change counter, useful as a memo/effect dependency. */
+ version: number;
+ /** Whether the active chat's panel is open. */
+ open: boolean;
+ /** The current display mode. */
+ mode: ChatMode;
+ /** The active registration, or undefined when none is registered. */
+ active: RegisteredChat | undefined;
+}
+
+/** Registration order is the singleton-resolution order: last entry wins. */
+const registrations: RegisteredChat[] = [];
+
+let panelOpen = false;
+let nextRegistrationId = 1;
+
+const registerEmitter = createEventEmitter<Chat>();
+const unregisterEmitter = createEventEmitter<Chat>();
+const openEmitter = createEventEmitter<Chat>();
+const closeEmitter = createEventEmitter<Chat>();
+const resizePanelEmitter = createEventEmitter<{ width: number }>();
+const modeEmitter = createEmitter<ChatMode>('floating');
+
+/**
+ * Host-internal: resolves the active chat with its providers.
+ * The most-recently-registered chat wins; when it is disposed the previous
+ * registration takes over the slot again.
+ */
+export const getActiveChat = (): RegisteredChat | undefined =>
+ registrations[registrations.length - 1];
+
+let snapshot: ChatSnapshot = {
+ version: 0,
+ open: false,
+ mode: modeEmitter.getCurrent(),
+ active: undefined,
+};
+
+const stateSubscribers = new Set<() => void>();
+
+const notifyState = () => {
+ snapshot = {
+ version: snapshot.version + 1,
+ open: panelOpen,
+ mode: modeEmitter.getCurrent(),
+ active: getActiveChat(),
+ };
+ stateSubscribers.forEach(fn => fn());
+};
+
+export const subscribeToChatState = (listener: () => void): (() => void) => {
+ stateSubscribers.add(listener);
+ return () => {
+ stateSubscribers.delete(listener);
+ };
+};
+
+export const getChatSnapshot = (): ChatSnapshot => snapshot;
+
+/** Closes the panel and fires `onDidClose` with the chat that was closed. */
+const closePanel = (closedChat: Chat) => {
+ panelOpen = false;
+ closeEmitter.fire(closedChat);
+};
+
+const registerChat: typeof chatApi.registerChat = (
+ chat: Chat,
+ trigger: () => ReactElement,
+ panel: () => ReactElement,
+): Disposable => {
+ const previousActive = getActiveChat();
+
+ // Re-registering an id replaces the previous entry and moves it to the
+ // most-recent position, mirroring the view registry's same-id semantics.
+ const existingIndex = registrations.findIndex(r => r.chat.id === chat.id);
+ if (existingIndex !== -1) {
+ registrations.splice(existingIndex, 1);
+ }
Review Comment:
**Suggestion:** Re-registering the same chat id removes the previous registration from the registry but does not emit `onDidUnregisterChat` for the displaced registration. Subscribers relying on unregister events for cleanup will miss that lifecycle transition and can keep stale state/resources. Emit the unregister event for the replaced registration when it is removed. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Extensions tracking chat lifecycle miss replacement unregistration.
- ⚠️ Resource cleanup tied to unregister leaks on chat upgrades.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe the chat host implementation in
`superset-frontend/src/core/chat/index.ts:137-186`, where `registerChat` manages the
`registrations` stack and exposes `onDidUnregisterChat` via `unregisterEmitter`.
2. In an extension (or a Jest test), subscribe to unregistration events using
`chat.onDidUnregisterChat` from the public API
(`superset-frontend/packages/superset-core/src/chat/index.ts:27-29`), implemented by
`onDidUnregisterChat: unregisterEmitter.event` at `src/core/chat/index.ts:225-227`.
3. Call `chat.registerChat({ id: 'acme.chat', name: 'Acme v1' }, trigger, panel)` and then
immediately call `chat.registerChat({ id: 'acme.chat', name: 'Acme v2' }, trigger, panel)`
again with the same `id`. The second call enters the same-id branch at
`src/core/chat/index.ts:146-149`, where it finds the existing index and `splice`s the
previous registration from `registrations` without firing `unregisterEmitter`.
4. Verify that the `onDidUnregisterChat` listener never runs for the displaced
registration: the only place that calls `unregisterEmitter.fire(chat)` is inside the
`Disposable` returned from `registerChat` at `src/core/chat/index.ts:169-185`, but in the
same-id re-registration case that `Disposable` later sees `index === -1` (because the
entry was already spliced at line 148) and returns early at line 173, so the previous
registration is effectively removed without any unregister event.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=24ab3ef20d9545938bf06728e3f1a86e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=24ab3ef20d9545938bf06728e3f1a86e&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/index.ts
**Line:** 146:149
**Comment:**
*Incomplete Implementation: Re-registering the same chat id removes the previous registration from the registry but does not emit `onDidUnregisterChat` for the displaced registration. Subscribers relying on unregister events for cleanup will miss that lifecycle transition and can keep stale state/resources. Emit the unregister event for the replaced registration when it is removed.
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%2F41006&comment_hash=c453bb192f89f8add4fd78b873d7079eff8eae9c7c14c7445c8ecf6b3eb8bb39&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41006&comment_hash=c453bb192f89f8add4fd78b873d7079eff8eae9c7c14c7445c8ecf6b3eb8bb39&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/chat/index.ts:
##########
@@ -0,0 +1,238 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * @fileoverview Host implementation of the `chat` contribution type.
+ *
+ * Chat is a dedicated contribution type, not a view: extensions register via
+ * the public `chat.registerChat()` and the host owns mounting, open/close
+ * state, and the display mode. Multiple chat extensions may register, but the
+ * host applies singleton resolution — the most-recently-registered chat is
+ * active; disposing it falls back to the previous one.
+ *
+ * Open-state policy across active-chat transitions: when the active chat's
+ * identity changes — a takeover by a different id, disposal falling back to a
+ * different id, or disposal of the last chat — the panel is closed (firing
+ * `onDidClose`) so the incoming chat never mounts into an open state it did
+ * not request. A same-id re-registration is an upgrade in place and keeps the
+ * open state.
+ *
+ * The public namespace (`chat`) is exposed to extensions on
+ * `window.superset`; the other exports are host-internal accessors for
+ * ChatMount and are NOT part of the public `@apache-superset/core` API.
+ */
+
+import { ReactElement } from 'react';
+import type { chat as chatApi } from '@apache-superset/core';
+import { Disposable } from '../models';
+import { createEmitter, createEventEmitter } from '../utils';
+
+type Chat = chatApi.Chat;
+type ChatMode = chatApi.ChatMode;
+
+/** A registered chat: its descriptor plus the host-mountable providers. */
+export interface RegisteredChat {
+ /** The chat descriptor passed to `registerChat`. */
+ chat: Chat;
+ /** Renders the collapsed bubble. Hidden by the host in panel mode. */
+ trigger: () => ReactElement;
+ /** Renders the chat panel, mounted per the current {@link ChatMode}. */
+ panel: () => ReactElement;
+ /**
+ * Unique per registration (a same-id re-registration gets a new one). The
+ * host UI keys mounts and fault containment on it, so a replacement resets
+ * crashed error boundaries instead of inheriting their latched state.
+ */
+ registrationId: number;
+}
+
+/**
+ * Immutable snapshot of the whole chat state, rebuilt on every change.
+ * Returned by reference from `getChatSnapshot` so `useSyncExternalStore`
+ * consumers read registrations, open state, and mode from one consistent
+ * object instead of tearing across separate live reads.
+ */
+export interface ChatSnapshot {
+ /** Monotonic change counter, useful as a memo/effect dependency. */
+ version: number;
+ /** Whether the active chat's panel is open. */
+ open: boolean;
+ /** The current display mode. */
+ mode: ChatMode;
+ /** The active registration, or undefined when none is registered. */
+ active: RegisteredChat | undefined;
+}
+
+/** Registration order is the singleton-resolution order: last entry wins. */
+const registrations: RegisteredChat[] = [];
+
+let panelOpen = false;
+let nextRegistrationId = 1;
+
+const registerEmitter = createEventEmitter<Chat>();
+const unregisterEmitter = createEventEmitter<Chat>();
+const openEmitter = createEventEmitter<Chat>();
+const closeEmitter = createEventEmitter<Chat>();
+const resizePanelEmitter = createEventEmitter<{ width: number }>();
+const modeEmitter = createEmitter<ChatMode>('floating');
+
+/**
+ * Host-internal: resolves the active chat with its providers.
+ * The most-recently-registered chat wins; when it is disposed the previous
+ * registration takes over the slot again.
+ */
+export const getActiveChat = (): RegisteredChat | undefined =>
+ registrations[registrations.length - 1];
+
+let snapshot: ChatSnapshot = {
+ version: 0,
+ open: false,
+ mode: modeEmitter.getCurrent(),
+ active: undefined,
+};
+
+const stateSubscribers = new Set<() => void>();
+
+const notifyState = () => {
+ snapshot = {
+ version: snapshot.version + 1,
+ open: panelOpen,
+ mode: modeEmitter.getCurrent(),
+ active: getActiveChat(),
+ };
+ stateSubscribers.forEach(fn => fn());
+};
+
+export const subscribeToChatState = (listener: () => void): (() => void) => {
+ stateSubscribers.add(listener);
+ return () => {
+ stateSubscribers.delete(listener);
+ };
+};
+
+export const getChatSnapshot = (): ChatSnapshot => snapshot;
+
+/** Closes the panel and fires `onDidClose` with the chat that was closed. */
+const closePanel = (closedChat: Chat) => {
+ panelOpen = false;
+ closeEmitter.fire(closedChat);
+};
+
+const registerChat: typeof chatApi.registerChat = (
+ chat: Chat,
+ trigger: () => ReactElement,
+ panel: () => ReactElement,
+): Disposable => {
+ const previousActive = getActiveChat();
+
+ // Re-registering an id replaces the previous entry and moves it to the
+ // most-recent position, mirroring the view registry's same-id semantics.
+ const existingIndex = registrations.findIndex(r => r.chat.id === chat.id);
+ if (existingIndex !== -1) {
+ registrations.splice(existingIndex, 1);
+ }
+
+ const entry: RegisteredChat = {
+ chat,
+ trigger,
+ panel,
+ registrationId: nextRegistrationId,
+ };
Review Comment:
**Suggestion:** The registry stores the incoming chat descriptor object by reference, so a caller can mutate that object after registration and change active chat metadata without any host event/state notification. This breaks snapshot immutability assumptions and can cause stale UI behavior. Store a copied descriptor when registering to decouple host state from caller-owned objects. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chat snapshot may mutate without state version increments.
- ⚠️ Extensions relying on immutable descriptors see inconsistent metadata.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. `registerChat` in `superset-frontend/src/core/chat/index.ts:137-159` builds a
`RegisteredChat` entry by assigning the caller-provided `chat` descriptor directly into
`entry.chat` (line 152) and pushing that entry into the `registrations` array (line 158).
2. An extension calls the public `dashboard.registerChat` API (declared in
`packages/superset-core/src/chat/index.ts:7-11`) with a mutable descriptor object, e.g.
`const desc = { id: 'acme.chat', name: 'Acme v1' }; chat.registerChat(desc, trigger,
panel);` — the host stores `desc` by reference in `registrations`.
3. Later, the extension mutates `desc`, e.g. `desc.name = 'Acme v2'`, without going
through any host API. Because `entry.chat` points to the same object, the active
registration's descriptor in `registrations` and in the current `snapshot.active` (rebuilt
via `notifyState` at `src/core/chat/index.ts:112-118`) changes in place, but no
`notifyState()` call is made and `snapshot.version` does not increment.
4. Consumers that rely on the snapshot being immutable — e.g. React code using
`useSyncExternalStore(subscribeToChatState, getChatSnapshot)` in
`superset-frontend/src/components/ChatMount/index.ts:47-55` or future extensions reading
`chat.getChat()` (which returns a shallow copy of `active.chat` at
`src/core/chat/index.ts:188-192`) — can observe descriptor fields changing without a
corresponding snapshot version bump or lifecycle event, breaking assumptions that
snapshots represent discrete, event-driven state changes.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9610281400704b1e93f642a7a705eb32&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9610281400704b1e93f642a7a705eb32&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/index.ts
**Line:** 151:156
**Comment:**
*Stale Reference: The registry stores the incoming chat descriptor object by reference, so a caller can mutate that object after registration and change active chat metadata without any host event/state notification. This breaks snapshot immutability assumptions and can cause stale UI behavior. Store a copied descriptor when registering to decouple host state from caller-owned objects.
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%2F41006&comment_hash=916af861c25569fc27ceede0b7e2623efc3ea37f5292feb5b6e393563d8cc4e5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41006&comment_hash=916af861c25569fc27ceede0b7e2623efc3ea37f5292feb5b6e393563d8cc4e5&reaction=dislike'>👎</a>
##########
superset-frontend/src/core/dashboard/index.ts:
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+
+/**
+ * Host-internal implementation of the `dashboard` namespace.
+ *
+ * Wraps Redux dashboardInfo and dataMask state and normalizes them into the
+ * stable `DashboardContext` contract. Extensions must not depend on the Redux
+ * slice structure directly.
+ */
+
+import type { dashboard as dashboardApi } from '@apache-superset/core';
+import type { DataMaskStateWithId } from '@superset-ui/core';
+import { HYDRATE_DASHBOARD } from 'src/dashboard/actions/hydrate';
+import {
+ UPDATE_DATA_MASK,
+ SET_DATA_MASK_FOR_FILTER_CHANGES_COMPLETE,
+} from 'src/dataMask/actions';
+import { store, RootState } from 'src/views/store';
+import { AnyListenerPredicate } from '@reduxjs/toolkit';
+import getChartIdsFromLayout from 'src/dashboard/util/getChartIdsFromLayout';
+import { createActionListener } from '../utils';
+import { navigation } from '../navigation';
+
+type DashboardContext = dashboardApi.DashboardContext;
+type FilterValue = dashboardApi.FilterValue;
+type ChartSummary = NonNullable<DashboardContext['charts']>[number];
+
+function buildChartSummaries(state: RootState): ChartSummary[] {
+ const slices = state.sliceEntities?.slices ?? {};
+ const layout = state.dashboardLayout?.present ?? {};
+
+ // Only charts actually placed on the dashboard layout — `slices` can also
+ // hold entities that are not on the current dashboard.
+ return getChartIdsFromLayout(layout).map(chartId => {
+ const slice = slices[chartId];
+ return {
+ chartId,
+ chartName: slice?.slice_name ?? '',
+ vizType: slice?.viz_type ?? '',
+ datasourceId: slice?.datasource_id ?? null,
+ datasourceName: slice?.datasource_name ?? null,
+ // Tab-accurate visibility is a deferred phase; every chart on the
+ // dashboard is reported visible for now.
+ isVisible: true,
+ };
+ });
+}
+
+function buildDashboardContext(): DashboardContext | undefined {
+ if (navigation.getPageType() !== 'dashboard') return undefined;
+ // `store.getState()` is already typed as RootState, so the slices below are
+ // read with their real types — the host owns this normalization and must
+ // stay type-safe against slice reshapes.
+ const state = store.getState();
+ const info = state.dashboardInfo;
+ if (!info?.id) return undefined;
+
+ const nativeFilters = state.nativeFilters?.filters ?? {};
+ const dataMask: DataMaskStateWithId = state.dataMask ?? {};
+
+ const filters: FilterValue[] = Object.entries(dataMask)
+ .filter(([id, mask]) => {
+ if (!(id in nativeFilters)) return false;
+ const value = mask?.filterState?.value;
+ return value !== null && value !== undefined;
+ })
+ .map(([id, mask]) => {
+ const raw = mask.filterState?.value;
+ return {
+ filterId: id,
+ label: nativeFilters[id]?.name ?? id,
+ value: Array.isArray(raw) ? [...raw] : raw,
+ };
Review Comment:
**Suggestion:** The returned filter `value` only clones arrays, but non-array object values are returned by reference from Redux state. If an extension mutates that object, it mutates store-backed state outside reducers and can corrupt dashboard filter state. Return a defensive copy for object values as well (for example via `structuredClone`/deep clone for plain objects). [stale reference]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Extensions can mutate Redux dataMask filter state directly.
- ⚠️ Corrupted filter state may break dashboard filtering behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. `buildDashboardContext` in `superset-frontend/src/core/dashboard/index.ts:66-99` reads
`dataMask` from the Redux store (`store.getState()` at line 71) and builds the exported
`filters` array from `Object.entries(dataMask)` at lines 78-91.
2. For each entry it assigns `const raw = mask.filterState?.value;` at line 85 and then
sets `value: Array.isArray(raw) ? [...raw] : raw` at line 89. Arrays are defensively
cloned, but any non-array object is passed through by reference into the
`FilterValue.value` returned to extensions.
3. Tests in `superset-frontend/src/core/dashboard/index.test.ts:22-31` explicitly verify
that array values are defensive copies by mutating `ctx.filters[0].value` and checking
that the underlying Redux `dataMask` array is unchanged, confirming the intent that
`FilterValue.value` must not allow direct mutation of store state.
4. If a filter's `filterState.value` is an object (e.g. a date range `{ start, end }` or a
complex selection), calling `dashboard.getCurrentDashboard()` (implemented at
`src/core/dashboard/index.ts:106-107`) returns a `DashboardContext` whose
`filters[0].value` is that same object reference; if an extension then mutates
`ctx.filters[0].value` (for example, `ctx.filters[0].value.start = '2024-01-01'`), the
Redux state at `store.getState().dataMask[filterId].filterState.value` will also change,
because no defensive clone is made for object values in `buildDashboardContext`.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f4c259d280fd4f51aeca9572cf08b3a5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f4c259d280fd4f51aeca9572cf08b3a5&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/dashboard/index.ts
**Line:** 85:90
**Comment:**
*Stale Reference: The returned filter `value` only clones arrays, but non-array object values are returned by reference from Redux state. If an extension mutates that object, it mutates store-backed state outside reducers and can corrupt dashboard filter state. Return a defensive copy for object values as well (for example via `structuredClone`/deep clone for plain objects).
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%2F41006&comment_hash=a095b43988a36a21952f3f0317be3f3bc48c2a54c326cd0b8521d5e72bc88fdc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41006&comment_hash=a095b43988a36a21952f3f0317be3f3bc48c2a54c326cd0b8521d5e72bc88fdc&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 dedicated chat contribution type (#41000) [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 #41006:
URL: https://github.com/apache/superset/pull/41006#discussion_r3405742091
##########
superset-frontend/src/components/ChatMount/index.tsx:
##########
@@ -0,0 +1,149 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { type ReactElement, useRef, useSyncExternalStore } from 'react';
+import { t } from '@apache-superset/core/translation';
+import { logging } from '@apache-superset/core/utils';
+import { css, useTheme } from '@apache-superset/core/theme';
+import { ErrorBoundary } from 'src/components/ErrorBoundary';
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+import { store } from 'src/views/store';
+import { getChatSnapshot, subscribeToChatState } from 'src/core/chat';
+
+const CHAT_EDGE_MARGIN = 24;
+const PANEL_MODE_WIDTH = 400;
+
+/**
+ * Wraps a chat provider in a React component so that ErrorBoundary can catch
+ * synchronous throws from the provider function itself. Calling `provider()`
+ * inline (e.g. `{activeChat.panel()}`) would throw outside React's render
+ * boundary and crash the host.
+ */
+const ChatRenderer = ({ provider }: { provider: () => ReactElement }) =>
+ provider();
+
+const ChatMount = () => {
+ const theme = useTheme();
+ // Notify at most once per registration; a crash can re-render and would
+ // otherwise re-toast, while a replacement (new registrationId) deserves a
+ // fresh notification if it crashes too.
+ const crashNotifiedFor = useRef<number | null>(null);
+
+ // The active chat, the open state, and the display mode are read from one
+ // immutable registry snapshot so a render never mixes state from two
+ // different store versions (the tearing useSyncExternalStore prevents).
+ const {
+ open: panelOpen,
+ mode,
+ active,
+ } = useSyncExternalStore(subscribeToChatState, getChatSnapshot);
+
+ if (!active) {
+ return null;
+ }
+
+ const { registrationId } = active;
+
+ const onProviderError = (error: Error) => {
+ // Fault isolation: contain the crash, log it, surface a one-time
+ // notification, and leave the slot empty rather than parking a
+ // persistent error card.
+ logging.error('[chat] provider crashed', error);
+ if (crashNotifiedFor.current !== registrationId) {
+ crashNotifiedFor.current = registrationId;
+ store.dispatch(addDangerToast(t('The chat failed to load.')));
+ }
+ };
+
+ if (mode === 'panel') {
+ // Panel mode hides the trigger and docks the panel to the right edge.
+ // Interim approximation of the "layout slot between header and footer"
+ // from the chat API contract — the dock overlays the page until the host
+ // grows a real layout slot and resizer chrome.
+ if (!panelOpen) {
+ return null;
+ }
+ return (
+ <div
+ data-test="chat-mount"
+ css={css`
+ position: fixed;
+ top: 0;
+ right: 0;
+ bottom: 0;
+ width: ${PANEL_MODE_WIDTH}px;
+ background: ${theme.colorBgContainer};
Review Comment:
**🎨 Design Review — MEDIUM**
Do you think a fixed 400px docked panel width might overflow or clip on narrow viewports (especially <400px), and should panel mode adapt at small breakpoints so the chat remains usable on smaller screens?
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1113468a0d1347588380e72e9dadba0d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1113468a0d1347588380e72e9dadba0d&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 **Design Review** comment — a question about the UX/design of frontend code. It is intentionally framed as a question, not a prescription. The author may agree or disagree.
**Path:** superset-frontend/src/components/ChatMount/index.tsx
**Line:** 89:90
**Comment:**
*MEDIUM: Do you think a fixed 400px docked panel width might overflow or clip on narrow viewports (especially <400px), and should panel mode adapt at small breakpoints so the chat remains usable on smaller screens?
- If you agree with the proposal, apply a small, localized change (swap a color token, bump a font size, adjust spacing, add an aria-label, etc.).
- If you disagree, or the answer depends on a design decision a human should make, explain your reasoning and ask the user how to proceed.
Do NOT refactor surrounding components or apply other design changes that weren't asked about.
```
</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): Introducing a Chat extension in Superset [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #41006:
URL: https://github.com/apache/superset/pull/41006#issuecomment-4744247801
Closed in favor of #41205, which includes all the changes from this 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