codeant-ai-for-open-source[bot] commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3481970695
##########
superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx:
##########
@@ -59,10 +72,16 @@ afterEach(() => {
}
} finally {
fetchMock.clearHistory().removeRoutes();
- jest.restoreAllMocks();
+ jest.clearAllMocks(); // Clears mock history but keeps spy in place
}
});
Review Comment:
**Suggestion:** These tests mock `window.location` via `jest.spyOn(...,
'get')` in individual cases, but the new cleanup only reassigns
`window.location` and calls `jest.clearAllMocks()`, which does not restore spy
wrappers. That leaks mocked location getters across tests and can cause
order-dependent failures. Restore the location spy(s) with `mockRestore()` (or
use `jest.restoreAllMocks()` in teardown). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Order-dependent Jest behavior when stacking window.location spies.
- ⚠️ Harder to safely add new window.location-based tests.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx`
and note the global Jest spy `const SupersetClientGet =
jest.spyOn(SupersetClient,
'get');` at lines 54-55 and the `let originalLocation: Location;` plus
`beforeEach`/`afterEach` block at lines 56-77.
2. Observe that some tests in this file (e.g. `test('should show missing
dataset state',
...)` at lines 18-27 of the tail section) call `jest.spyOn(window,
'location',
'get').mockReturnValue(...)` to override the `window.location` getter
without capturing or
restoring the spy instance.
3. In `afterEach` (lines 62-76) the code reassigns `window.location =
originalLocation;`
and calls `jest.clearAllMocks()`, which clears mock histories and
implementations but does
not call `mockRestore()` on the accessor spy created by `jest.spyOn(window,
'location',
'get')`, so the patched getter descriptor remains installed on
`window.location` after the
test.
4. When this suite runs in the same Jest worker as other tests that also spy
on
`window.location` (for example `ExploreViewContainer.test.tsx:142` or
`ChartCreation.test.tsx:248, 278, 315`, which each call `jest.spyOn(window,
'location',
'get')`), those tests now interact with an already-spied accessor, creating
order-dependent behavior and making it possible for double-spy attempts or
descriptor
assumptions to fail unless `mockRestore()`/`jest.restoreAllMocks()` is used.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=20361b78884c43af9538c33806e90583&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=20361b78884c43af9538c33806e90583&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/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx
**Line:** 56:77
**Comment:**
*Logic Error: These tests mock `window.location` via `jest.spyOn(...,
'get')` in individual cases, but the new cleanup only reassigns
`window.location` and calls `jest.clearAllMocks()`, which does not restore spy
wrappers. That leaks mocked location getters across tests and can cause
order-dependent failures. Restore the location spy(s) with `mockRestore()` (or
use `jest.restoreAllMocks()` in teardown).
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%2F39461&comment_hash=5820d78a57df7bc43067f874f7e1d5de19222b2c69df2f8d69828350cd76e55b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=5820d78a57df7bc43067f874f7e1d5de19222b2c69df2f8d69828350cd76e55b&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx:
##########
@@ -237,413 +217,388 @@ const preventRouterLinkWhileMetaClicked = (evt:
React.MouseEvent) => {
}
};
-class DatasourceControl extends PureComponent<
- DatasourceControlProps,
- DatasourceControlState
-> {
- static defaultProps = defaultProps;
-
- constructor(props: DatasourceControlProps) {
- super(props);
- this.state = {
- showEditDatasourceModal: false,
- showChangeDatasourceModal: false,
- showSaveDatasetModal: false,
- };
- }
-
- onDatasourceSave = (datasource: Datasource) => {
- // Cast to ExtendedDatasource for the component's internal use
- this.props.actions.changeDatasource(datasource as ExtendedDatasource);
- // Cast datasource for getTemporalColumns which expects Dataset |
QueryResponse
- const { temporalColumns, defaultTemporalColumn } = getTemporalColumns(
- datasource as Parameters<typeof getTemporalColumns>[0],
- );
- const { columns } = datasource;
- // the current granularity_sqla might not be a temporal column anymore
- const timeCol = this.props.form_data?.granularity_sqla;
- const isGranularitySqlaTemporal = columns.find(
- ({ column_name }) => column_name === timeCol,
- )?.is_dttm;
- // the current main_dttm_col might not be a temporal column anymore
- const isDefaultTemporal = columns.find(
- ({ column_name }) => column_name === defaultTemporalColumn,
- )?.is_dttm;
-
- // if the current granularity_sqla is empty or it is not a temporal column
anymore
- // let's update the control value
- if (datasource.type === 'table' && !isGranularitySqlaTemporal) {
- const temporalColumn = isDefaultTemporal
- ? defaultTemporalColumn
- : temporalColumns?.[0];
- this.props.actions.setControlValue(
- 'granularity_sqla',
- temporalColumn || null,
+export default function DatasourceControl({
+ actions,
+ onChange = () => {},
+ value = null,
+ datasource,
+ form_data,
+ isEditable = true,
+ onDatasourceSave = null,
+ user,
+}: DatasourceControlProps) {
+ const theme = useTheme();
+
+ const [showEditDatasourceModal, setShowEditDatasourceModal] =
useState(false);
+ const [showChangeDatasourceModal, setShowChangeDatasourceModal] =
+ useState(false);
+ const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
+
+ const handleDatasourceSave = useCallback(
+ (savedDatasource: Datasource) => {
+ // Cast to ExtendedDatasource for the component's internal use
+ actions.changeDatasource(savedDatasource as ExtendedDatasource);
+ // Cast datasource for getTemporalColumns which expects Dataset |
QueryResponse
+ const { temporalColumns, defaultTemporalColumn } = getTemporalColumns(
+ savedDatasource as Parameters<typeof getTemporalColumns>[0],
);
- }
+ const { columns } = savedDatasource;
+ // the granularity_sqla might not be a temporal column anymore
+ const timeCol = form_data?.granularity_sqla;
+ const isGranularitySqlaTemporal = columns.find(
+ ({ column_name }) => column_name === timeCol,
+ )?.is_dttm;
+ // the main_dttm_col might not be a temporal column anymore
+ const isDefaultTemporal = columns.find(
+ ({ column_name }) => column_name === defaultTemporalColumn,
+ )?.is_dttm;
+
+ // if granularity_sqla is empty or it is not a temporal column anymore
+ // let's update the control value
+ if (savedDatasource.type === 'table' && !isGranularitySqlaTemporal) {
+ const temporalColumn = isDefaultTemporal
+ ? defaultTemporalColumn
+ : temporalColumns?.[0];
+ actions.setControlValue('granularity_sqla', temporalColumn || null);
+ }
- if (this.props.onDatasourceSave) {
- this.props.onDatasourceSave(datasource);
- }
- };
+ if (onDatasourceSave) {
+ onDatasourceSave(savedDatasource);
+ }
+ },
+ [actions, form_data?.granularity_sqla, onDatasourceSave],
+ );
- toggleShowDatasource = () => {
- this.setState(({ showDatasource }) => ({
- showDatasource: !showDatasource,
- }));
- };
+ const toggleChangeDatasourceModal = useCallback(() => {
+ setShowChangeDatasourceModal(prev => !prev);
+ }, []);
+
+ const toggleEditDatasourceModal = useCallback(() => {
+ setShowEditDatasourceModal(prev => !prev);
+ }, []);
+
+ const toggleSaveDatasetModal = useCallback(() => {
+ setShowSaveDatasetModal(prev => !prev);
+ }, []);
+
+ const handleMenuItemClick = useCallback(
+ ({ key }: { key: string }) => {
+ switch (key) {
+ case CHANGE_DATASET:
+ toggleChangeDatasourceModal();
+ break;
+
+ case EDIT_DATASET:
+ toggleEditDatasourceModal();
+ break;
+
+ case VIEW_IN_SQL_LAB:
+ {
+ const payload = {
+ datasourceKey: `${datasource.id}__${datasource.type}`,
+ sql: datasource.sql,
+ };
+ SupersetClient.postForm('/sqllab/', {
+ form_data: safeStringify(payload),
+ });
+ }
+ break;
+
+ case SAVE_AS_DATASET:
+ toggleSaveDatasetModal();
+ break;
+
+ default:
+ break;
+ }
+ },
+ [
+ datasource,
+ toggleChangeDatasourceModal,
+ toggleEditDatasourceModal,
+ toggleSaveDatasetModal,
+ ],
+ );
- toggleChangeDatasourceModal = () => {
- this.setState(({ showChangeDatasourceModal }) => ({
- showChangeDatasourceModal: !showChangeDatasourceModal,
- }));
- };
+ let extra;
+ if (datasource?.extra) {
+ if (typeof datasource.extra === 'string') {
+ try {
+ extra = JSON.parse(datasource.extra);
+ } catch {} // eslint-disable-line no-empty
+ } else {
+ extra = datasource.extra; // eslint-disable-line prefer-destructuring
+ }
+ }
+ const isMissingDatasource = !datasource?.id || Boolean(extra?.error);
+ let isMissingParams = false;
+ if (isMissingDatasource) {
+ const datasourceId = getUrlParam(URL_PARAMS.datasourceId);
+ const sliceId = getUrlParam(URL_PARAMS.sliceId);
+
+ if (!datasourceId && !sliceId) {
+ isMissingParams = true;
+ }
+ }
- toggleEditDatasourceModal = () => {
- this.setState(({ showEditDatasourceModal }) => ({
- showEditDatasourceModal: !showEditDatasourceModal,
- }));
- };
+ const allowEdit =
+ datasource.owners?.map(o => o.id || o.value).includes(user.userId) ||
+ isUserAdmin(user);
- toggleSaveDatasetModal = () => {
- this.setState(({ showSaveDatasetModal }) => ({
- showSaveDatasetModal: !showSaveDatasetModal,
- }));
- };
+ const canAccessSqlLab = userHasPermission(user, 'SQL Lab', 'menu_access');
- handleMenuItemClick = ({ key }: { key: string }) => {
- switch (key) {
- case CHANGE_DATASET:
- this.toggleChangeDatasourceModal();
- break;
-
- case EDIT_DATASET:
- this.toggleEditDatasourceModal();
- break;
-
- case VIEW_IN_SQL_LAB:
- {
- const { datasource } = this.props;
- const payload = {
- datasourceKey: `${datasource.id}__${datasource.type}`,
- sql: datasource.sql,
- };
- SupersetClient.postForm('/sqllab/', {
- form_data: safeStringify(payload),
- });
- }
- break;
-
- case SAVE_AS_DATASET:
- this.toggleSaveDatasetModal();
- break;
-
- default:
- break;
- }
+ const editText = t('Edit %s', datasetLabelLower());
+ const requestedQuery = {
+ datasourceKey: `${datasource.id}__${datasource.type}`,
+ sql: datasource.sql,
};
+ const defaultDatasourceMenuItems = [];
+ if (isEditable && !isMissingDatasource) {
+ defaultDatasourceMenuItems.push({
+ key: EDIT_DATASET,
+ label: !allowEdit ? (
+ <Tooltip
+ title={t(
+ 'You must be a %s owner in order to edit. Please reach out to a %s
owner to request modifications or edit access.',
+ datasetLabelLower(),
+ datasetLabelLower(),
+ )}
+ >
+ {editText}
+ </Tooltip>
+ ) : (
+ editText
+ ),
+ disabled: !allowEdit,
+ 'data-test': 'edit-dataset',
+ });
+ }
- render() {
- const {
- showChangeDatasourceModal,
- showEditDatasourceModal,
- showSaveDatasetModal,
- } = this.state;
- const { datasource, onChange, theme } = this.props;
- let extra;
- if (datasource?.extra) {
- if (typeof datasource.extra === 'string') {
- try {
- extra = JSON.parse(datasource.extra);
- } catch {} // eslint-disable-line no-empty
- } else {
- extra = datasource.extra; // eslint-disable-line prefer-destructuring
- }
- }
- const isMissingDatasource = !datasource?.id || Boolean(extra?.error);
- let isMissingParams = false;
- if (isMissingDatasource) {
- const datasourceId = getUrlParam(URL_PARAMS.datasourceId);
- const sliceId = getUrlParam(URL_PARAMS.sliceId);
-
- if (!datasourceId && !sliceId) {
- isMissingParams = true;
- }
- }
-
- const { user } = this.props;
- const allowEdit =
- datasource.owners?.map(o => o.id || o.value).includes(user.userId) ||
- isUserAdmin(user);
-
- const canAccessSqlLab = userHasPermission(user, 'SQL Lab', 'menu_access');
-
- const editText = t('Edit %s', datasetLabelLower());
- const requestedQuery = {
- datasourceKey: `${datasource.id}__${datasource.type}`,
- sql: datasource.sql,
- };
- const defaultDatasourceMenuItems = [];
- if (this.props.isEditable && !isMissingDatasource) {
- defaultDatasourceMenuItems.push({
- key: EDIT_DATASET,
- label: !allowEdit ? (
- <Tooltip
- title={t(
- 'You must be a %s owner in order to edit. Please reach out to a
%s owner to request modifications or edit access.',
- datasetLabelLower(),
- datasetLabelLower(),
- )}
- >
- {editText}
- </Tooltip>
- ) : (
- editText
- ),
- disabled: !allowEdit,
- 'data-test': 'edit-dataset',
- });
- }
+ defaultDatasourceMenuItems.push({
+ key: CHANGE_DATASET,
+ label: t('Swap %s', datasetLabelLower()),
+ });
+ if (!isMissingDatasource && canAccessSqlLab) {
defaultDatasourceMenuItems.push({
- key: CHANGE_DATASET,
- label: t('Swap %s', datasetLabelLower()),
+ key: VIEW_IN_SQL_LAB,
+ label: (
+ <Link
+ to={{
+ pathname: '/sqllab',
+ state: { requestedQuery },
+ }}
+ onClick={preventRouterLinkWhileMetaClicked}
+ >
+ {t('View in SQL Lab')}
+ </Link>
+ ),
});
+ }
- if (!isMissingDatasource && canAccessSqlLab) {
- defaultDatasourceMenuItems.push({
- key: VIEW_IN_SQL_LAB,
- label: (
- <Link
- to={{
- pathname: '/sqllab',
- state: { requestedQuery },
- }}
- onClick={preventRouterLinkWhileMetaClicked}
- >
- {t('View in SQL Lab')}
- </Link>
- ),
- });
- }
-
- const defaultDatasourceMenu = (
- <Menu
- onClick={this.handleMenuItemClick}
- items={defaultDatasourceMenuItems}
- />
- );
-
- const queryDatasourceMenuItems = [
- {
- key: QUERY_PREVIEW,
- label: (
- <ModalTrigger
- triggerNode={
- <div data-test="view-query-menu-item">{t('Query preview')}</div>
- }
- modalTitle={t('Query preview')}
- modalBody={
- <ViewQuery
- sql={datasource?.sql || datasource?.select_star || ''}
- datasource={`${datasource.id}__${datasource.type}`}
- />
- }
- modalFooter={
- <ViewQueryModalFooter
- changeDatasource={this.toggleSaveDatasetModal}
- datasource={{
- id: String(datasource.id),
- sql: datasource.sql || '',
- type: datasource.type,
- }}
- />
- }
- draggable={false}
- resizable={false}
- responsive
- />
- ),
- },
- ];
-
- if (canAccessSqlLab) {
- queryDatasourceMenuItems.push({
- key: VIEW_IN_SQL_LAB,
- label: (
- <Link
- to={{
- pathname: '/sqllab',
- state: { requestedQuery },
- }}
- onClick={preventRouterLinkWhileMetaClicked}
- >
- {t('View in SQL Lab')}
- </Link>
- ),
- });
- }
+ const defaultDatasourceMenu = (
+ <Menu onClick={handleMenuItemClick} items={defaultDatasourceMenuItems} />
+ );
+ const queryDatasourceMenuItems = [
+ {
+ key: QUERY_PREVIEW,
+ label: (
+ <ModalTrigger
+ triggerNode={
+ <div data-test="view-query-menu-item">{t('Query preview')}</div>
+ }
+ modalTitle={t('Query preview')}
+ modalBody={
+ <ViewQuery
+ sql={datasource?.sql || datasource?.select_star || ''}
+ datasource={`${datasource.id}__${datasource.type}`}
+ />
+ }
+ modalFooter={
+ <ViewQueryModalFooter
+ changeDatasource={toggleSaveDatasetModal}
+ datasource={{
+ id: String(datasource.id),
+ sql: datasource.sql || '',
+ type: datasource.type,
+ }}
+ />
+ }
+ draggable={false}
+ resizable={false}
+ responsive
+ />
+ ),
+ },
+ ];
+
+ if (canAccessSqlLab) {
queryDatasourceMenuItems.push({
- key: SAVE_AS_DATASET,
- label: <span>{t('Save as %s', datasetLabelLower())}</span>,
+ key: VIEW_IN_SQL_LAB,
+ label: (
+ <Link
+ to={{
+ pathname: '/sqllab',
+ state: { requestedQuery },
+ }}
+ onClick={preventRouterLinkWhileMetaClicked}
+ >
+ {t('View in SQL Lab')}
+ </Link>
+ ),
});
+ }
- const queryDatasourceMenu = (
- <Menu
- onClick={this.handleMenuItemClick}
- items={queryDatasourceMenuItems}
- />
- );
-
- const { health_check_message: healthCheckMessage } = datasource;
-
- const titleText =
- isMissingDatasource && !datasource.name
- ? t('Missing %s', datasetLabelLower())
- : getDatasourceTitle(datasource);
-
- const tooltip = titleText;
-
- return (
- <Styles data-test="datasource-control" className="DatasourceControl">
- <div className="data-container">
- {datasourceIconLookup[getDatasetType(datasource)]}
- {renderDatasourceTitle(titleText, tooltip)}
- {healthCheckMessage && (
- <Tooltip title={healthCheckMessage}>
- <Icons.WarningOutlined
- css={css`
- margin-left: ${theme.sizeUnit * 2}px;
- `}
- iconColor={theme.colorWarning}
- />
- </Tooltip>
- )}
- {extra?.warning_markdown && (
- <WarningIconWithTooltip warningMarkdown={extra.warning_markdown} />
- )}
- <Dropdown
- popupRender={() =>
- datasource.type === DatasourceType.Query
- ? queryDatasourceMenu
- : defaultDatasourceMenu
- }
- trigger={['click']}
- data-test="datasource-menu"
- >
- <Icons.MoreOutlined
- iconSize="xl"
- iconColor={theme.colorPrimary}
- className="datasource-modal-trigger"
- data-test="datasource-menu-trigger"
- />
- </Dropdown>
- </div>
- {/* missing dataset */}
- {isMissingDatasource && isMissingParams && (
- <div className="error-alert">
- <ErrorAlert
- type="warning"
- message={t('Missing URL parameters')}
- description={t(
- 'The URL is missing the dataset_id or slice_id parameters.',
- )}
+ queryDatasourceMenuItems.push({
+ key: SAVE_AS_DATASET,
+ label: <span>{t('Save as %s', datasetLabelLower())}</span>,
+ });
+
+ const queryDatasourceMenu = (
+ <Menu onClick={handleMenuItemClick} items={queryDatasourceMenuItems} />
+ );
+
+ const { health_check_message: healthCheckMessage } = datasource;
+
+ const titleText =
+ isMissingDatasource && !datasource.name
+ ? t('Missing %s', datasetLabelLower())
+ : getDatasourceTitle(datasource);
+
+ const tooltip = titleText;
+
+ return (
+ <Styles data-test="datasource-control" className="DatasourceControl">
+ <div className="data-container">
+ {datasourceIconLookup[getDatasetType(datasource)]}
+ {renderDatasourceTitle(titleText, tooltip)}
+ {healthCheckMessage && (
+ <Tooltip title={healthCheckMessage}>
+ <Icons.WarningOutlined
+ css={css`
+ margin-left: ${theme.sizeUnit * 2}px;
+ `}
+ iconColor={theme.colorWarning}
/>
- </div>
+ </Tooltip>
)}
- {isMissingDatasource && !isMissingParams && (
- <div className="error-alert">
- {extra?.error ? (
- <ErrorMessageWithStackTrace
- title={extra.error.statusText || extra.error.message}
- subtitle={
- extra.error.statusText ? extra.error.message : undefined
- }
- error={extra.error}
- source="explore"
- />
- ) : (
- <ErrorAlert
- type="warning"
- message={t('Missing %s', datasetLabelLower())}
- descriptionPre={false}
- descriptionDetailsCollapsed={false}
- descriptionDetails={
- <>
- <p>
- {t(
- 'The %s linked to this chart may have been deleted.',
- datasetLabelLower(),
- )}
- </p>
- <p>
- <Button
- buttonStyle="primary"
- onClick={() =>
- this.handleMenuItemClick({ key: CHANGE_DATASET })
- }
- >
- {t('Swap %s', datasetLabelLower())}
- </Button>
- </p>
- </>
- }
- />
- )}
- </div>
+ {extra?.warning_markdown && (
+ <WarningIconWithTooltip warningMarkdown={extra.warning_markdown} />
)}
- {showEditDatasourceModal &&
- (String(datasource.type) === 'semantic_view' ? (
- <SemanticViewEditModal
- show={showEditDatasourceModal}
- onHide={this.toggleEditDatasourceModal}
- onSave={() => this.onDatasourceSave(datasource)}
- semanticView={{
- id: datasource.id,
- table_name: datasource.name,
- description: datasource.description,
- cache_timeout: datasource.cache_timeout,
- }}
+ <Dropdown
+ popupRender={() =>
+ datasource.type === DatasourceType.Query
+ ? queryDatasourceMenu
+ : defaultDatasourceMenu
+ }
+ trigger={['click']}
+ data-test="datasource-menu"
+ >
+ <Icons.MoreOutlined
+ iconSize="xl"
+ iconColor={theme.colorPrimary}
+ className="datasource-modal-trigger"
+ data-test="datasource-menu-trigger"
+ />
+ </Dropdown>
+ </div>
+ {/* missing dataset */}
+ {isMissingDatasource && isMissingParams && (
+ <div className="error-alert">
+ <ErrorAlert
+ type="warning"
+ message={t('Missing URL parameters')}
+ description={t(
+ 'The URL is missing the dataset_id or slice_id parameters.',
+ )}
+ />
+ </div>
+ )}
+ {isMissingDatasource && !isMissingParams && (
+ <div className="error-alert">
+ {extra?.error ? (
+ <ErrorMessageWithStackTrace
+ title={extra.error.statusText || extra.error.message}
+ subtitle={
+ extra.error.statusText ? extra.error.message : undefined
+ }
+ error={extra.error}
+ source="explore"
/>
) : (
- <DatasourceModal
- datasource={datasource}
- show={showEditDatasourceModal}
- onDatasourceSave={this.onDatasourceSave}
- onHide={this.toggleEditDatasourceModal}
+ <ErrorAlert
+ type="warning"
+ message={t('Missing %s', datasetLabelLower())}
+ descriptionPre={false}
+ descriptionDetailsCollapsed={false}
+ descriptionDetails={
+ <>
+ <p>
+ {t(
+ 'The %s linked to this chart may have been deleted.',
+ datasetLabelLower(),
+ )}
+ </p>
+ <p>
+ <Button
+ buttonStyle="primary"
+ onClick={() =>
+ handleMenuItemClick({ key: CHANGE_DATASET })
+ }
+ >
+ {t('Swap %s', datasetLabelLower())}
+ </Button>
+ </p>
+ </>
+ }
/>
- ))}
- {showChangeDatasourceModal && (
- <ChangeDatasourceModal
- onDatasourceSave={this.onDatasourceSave}
- onHide={this.toggleChangeDatasourceModal}
- show={showChangeDatasourceModal}
- onChange={onChange}
+ )}
+ </div>
+ )}
+ {showEditDatasourceModal &&
+ (String(datasource.type) === 'semantic_view' ? (
+ <SemanticViewEditModal
+ show={showEditDatasourceModal}
+ onHide={toggleEditDatasourceModal}
+ onSave={() => handleDatasourceSave(datasource)}
+ semanticView={{
+ id: datasource.id,
+ table_name: datasource.name,
+ description: datasource.description,
+ cache_timeout: datasource.cache_timeout,
+ }}
/>
Review Comment:
**Suggestion:** The semantic-view save path calls the generic
datasource-save handler with the original `datasource` object, so edited fields
(like description/cache timeout) are never propagated to `changeDatasource` or
`onDatasourceSave`. This causes stale datasource state after saving. Pass the
updated semantic-view payload (or refetch and pass fresh data) instead of the
pre-edit object. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Semantic view edits not reflected in Explore datasource state.
- ⚠️ Callers of onDatasourceSave see stale semantic-view metadata.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/src/explore/components/controls/DatasourceControl/index.tsx`,
note the semantic-view edit branch at lines 560-571: when
`showEditDatasourceModal` is
true and `String(datasource.type) === 'semantic_view'`, the component renders
`<SemanticViewEditModal ... />` with `onSave={() =>
handleDatasourceSave(datasource)}` and
a `semanticView` prop built from `datasource.id`, `datasource.name`,
`datasource.description`, and `datasource.cache_timeout`.
2. Inspect `handleDatasourceSave` at lines 237-268 in the same file: it calls
`actions.changeDatasource(savedDatasource as ExtendedDatasource)`, adjusts
temporal column
controls via `getTemporalColumns(savedDatasource)`, and then invokes the
optional
`onDatasourceSave(savedDatasource)` callback with whatever `savedDatasource`
argument it
receives.
3. Open
`superset-frontend/src/features/semanticViews/SemanticViewEditModal.tsx` and
observe `SemanticViewEditModal`'s `handleSave` function at lines 85-98: on
save it issues
a `SupersetClient.put` to `/api/v1/semantic_view/${semanticView.id}`, then
calls the
`onSave` callback (with no arguments) and `onHide()` once the backend update
succeeds.
4. When a user edits a semantic view's description/cache timeout from
Explore (so
`DatasourceControl` receives a `datasource` with `type === 'semantic_view'`
and
`showEditDatasourceModal` is toggled), saving changes triggers
`SemanticViewEditModal`'s
`onSave`, which in turn calls `handleDatasourceSave(datasource)` with the
original
`datasource` object captured in the closure; this means
`actions.changeDatasource` and any
`onDatasourceSave` handler are invoked with stale
`description`/`cache_timeout` values, so
the Explore datasource state and any consumers of `onDatasourceSave` never
see the updated
semantic-view metadata until a full refetch or page reload.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d7249941939447cf8979be6765cd6f51&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=d7249941939447cf8979be6765cd6f51&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/explore/components/controls/DatasourceControl/index.tsx
**Line:** 561:571
**Comment:**
*Incomplete Implementation: The semantic-view save path calls the
generic datasource-save handler with the original `datasource` object, so
edited fields (like description/cache timeout) are never propagated to
`changeDatasource` or `onDatasourceSave`. This causes stale datasource state
after saving. Pass the updated semantic-view payload (or refetch and pass fresh
data) instead of the pre-edit object.
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%2F39461&comment_hash=275686efacdd8aa7d525a1058d9e2b182fbbb93ce61995fbcd2e5e00ee6b39ce&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=275686efacdd8aa7d525a1058d9e2b182fbbb93ce61995fbcd2e5e00ee6b39ce&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
