codeant-ai-for-open-source[bot] commented on code in PR #43769:
URL: https://github.com/apache/superset/pull/43769#discussion_r3922548608
##########
superset/daos/chart.py:
##########
@@ -166,3 +167,33 @@ def remove_favorite(chart: Slice) -> None:
)
if fav:
db.session.delete(fav)
+
+
+class EmbeddedChartDAO(BaseDAO[EmbeddedChart]):
+ # There isn't really a regular scenario where we would rather get Embedded
by id
+ id_column_name = "uuid"
+
+ @staticmethod
+ def upsert(chart: Slice, allowed_domains: list[str]) -> EmbeddedChart:
+ """
+ Sets up a chart to be embeddable.
+ Upsert is used to preserve the embedded_chart uuid across updates.
+ """
+ embedded: EmbeddedChart = (
+ chart.embedded[0] if chart.embedded else EmbeddedChart()
+ )
Review Comment:
**Suggestion:** Concurrent requests can both observe an empty
`chart.embedded` collection and create separate rows because `slice_id` is not
unique, making the selected embed UUID and domains nondeterministic. [race
condition]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7dedb049d94b4af99adddc1c167e660a&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=7dedb049d94b4af99adddc1c167e660a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/chart.py
**Line:** 182:184
**Comment:**
*Race Condition: Concurrent requests can both observe an empty
`chart.embedded` collection and create separate rows because `slice_id` is not
unique, making the selected embed UUID and domains nondeterministic.
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%2F43769&comment_hash=681353d6525b75cb6254d0efe66e3c77154fcd0ea6648ec4261de0feeca8b2d9&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43769&comment_hash=681353d6525b75cb6254d0efe66e3c77154fcd0ea6648ec4261de0feeca8b2d9&reaction=dislike'>๐</a>
##########
superset/charts/api.py:
##########
@@ -1874,3 +1897,181 @@ def restore_version(self, uuid_str: str,
version_uuid_str: str) -> Response:
return restore_version_endpoint(
self, Slice, RestoreChartVersionCommand, uuid_str, version_uuid_str
)
+
+ @expose("/<pk>/embedded", methods=("GET",))
+ @protect()
+ @safe
+ @permission_name("read")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.get_embedded",
+ log_to_statsd=False,
+ )
+ def get_embedded(self, pk: int) -> Response:
+ """Get the chart's embedded configuration.
+ ---
+ get:
+ summary: Get the chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The chart id
+ responses:
+ 200:
+ description: Result contains the embedded chart config
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref:
'#/components/schemas/EmbeddedChartResponseSchema'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = ChartDAO.find_by_id(pk)
+ if not chart:
+ return self.response_404()
+ if not chart.embedded:
+ return self.response(404)
+ embedded: EmbeddedChart = chart.embedded[0]
+ result = self.embedded_response_schema.dump(embedded)
+ return self.response(200, result=result)
+
+ @expose("/<pk>/embedded", methods=("POST", "PUT"))
+ @protect()
+ @safe
+ @permission_name("set_embedded")
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.set_embedded",
+ log_to_statsd=False,
+ )
+ def set_embedded(self, pk: int) -> Response:
+ """Set a chart's embedded configuration.
+ ---
+ post:
+ summary: Set a chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The chart id
+ requestBody:
+ description: The embedded configuration to set
+ required: true
+ content:
+ application/json:
+ schema: EmbeddedChartConfigSchema
+ responses:
+ 200:
+ description: Successfully set the configuration
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref:
'#/components/schemas/EmbeddedChartResponseSchema'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ put:
+ summary: Update a chart's embedded configuration
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The chart id
+ requestBody:
+ description: The embedded configuration to set
+ required: true
+ content:
+ application/json:
+ schema: EmbeddedChartConfigSchema
+ responses:
+ 200:
+ description: Successfully set the configuration
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ $ref:
'#/components/schemas/EmbeddedChartResponseSchema'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ chart = ChartDAO.find_by_id(pk)
+ if not chart:
+ return self.response_404()
+ try:
+ body = self.embedded_config_schema.load(request.json)
+ embedded = EmbeddedChartDAO.upsert(chart, body["allowed_domains"])
+ db.session.commit() # pylint: disable=consider-using-transaction
+ result = self.embedded_response_schema.dump(embedded)
Review Comment:
**Suggestion:** The response schema declares `chart_id`, but `EmbeddedChart`
exposes `slice_id`; dumping this object omits the chart identifier from every
embedded-configuration response. [api mismatch]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Often`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7b13b40b3f794cc48b0157ee84252594&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=7b13b40b3f794cc48b0157ee84252594&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/api.py
**Line:** 2027:2027
**Comment:**
*Api Mismatch: The response schema declares `chart_id`, but
`EmbeddedChart` exposes `slice_id`; dumping this object omits the chart
identifier from every embedded-configuration response.
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%2F43769&comment_hash=c05fa79aee5273a5bc7d342499b6c09e5a7c64c38d71ea2112757d3d03a962af&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43769&comment_hash=c05fa79aee5273a5bc7d342499b6c09e5a7c64c38d71ea2112757d3d03a962af&reaction=dislike'>๐</a>
##########
superset-frontend/src/embedded/embeddedChart/hydrateEmbedded.ts:
##########
@@ -0,0 +1,163 @@
+/**
+ * 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 { DataMaskWithId, JsonObject } from '@superset-ui/core';
+import { chart } from 'src/components/Chart/chartReducer';
+import { getInitialDataMask } from 'src/dataMask/reducer';
+import { applyDefaultFormData } from 'src/explore/store';
+import { CommonBootstrapData } from 'src/types/bootstrapTypes';
+import { HYDRATE_DASHBOARD } from 'src/dashboard/actions/hydrate';
+import { Datasource } from 'src/dashboard/types';
+import {
+ DASHBOARD_ROOT_ID,
+ DASHBOARD_GRID_ID,
+} from 'src/dashboard/util/constants';
+import {
+ DASHBOARD_ROOT_TYPE,
+ DASHBOARD_GRID_TYPE,
+} from 'src/dashboard/util/componentTypes';
+
+/**
+ * A chart embedded on its own still renders through the dashboard's chart
+ * stack, because that is where cross-filtering, drill, and the header controls
+ * live. Rather than reimplement any of that, this builds the minimum slice of
+ * dashboard state a single chart needs and lets the existing components run
+ * against it unchanged.
+ *
+ * It reuses HYDRATE_DASHBOARD rather than introducing a parallel action, so
+ * every dashboard reducer stays untouched: `charts`, `sliceEntities`,
+ * `dataMask`, `dashboardInfo` and `dashboardState` all already handle it.
+ * `dashboardLayout` and `nativeFilters` handle it too but dereference their
+ * slice unconditionally, so the payload carries an empty stand-in for each.
+ * `datasources` is the one slice with no hydrate handler at all, so the caller
+ * dispatches `setDatasources` for it separately.
+ *
+ * Every slice any HYDRATE_DASHBOARD handler reads has to appear here; the
+ * accompanying test asserts that, because a missing one only fails at runtime
+ * and only in the embedded path.
+ */
+
+export interface EmbeddedChartData {
+ slice: {
+ slice_id: number;
+ slice_url: string;
+ slice_name: string;
+ form_data: JsonObject & { viz_type: string; datasource: string };
+ description?: string | null;
+ description_markeddown?: string | null;
+ modified?: string | null;
+ changed_on?: string | number | null;
+ };
+ // The explore endpoint returns the full datasource, and `setDatasources`
+ // stores it as one, so it is typed as such rather than loosely.
+ dataset: Datasource;
+}
+
+export interface HydrateEmbeddedAction {
+ type: typeof HYDRATE_DASHBOARD;
+ data: {
+ charts: Record<number, JsonObject>;
+ sliceEntities: { slices: Record<number, JsonObject> };
+ dataMask: Record<number, DataMaskWithId>;
+ dashboardInfo: JsonObject;
+ dashboardState: JsonObject;
+ dashboardLayout: { present: JsonObject };
+ nativeFilters: { filters: JsonObject };
+ };
+}
+
+const hydrateEmbedded = (
+ { slice }: EmbeddedChartData,
+ common: CommonBootstrapData,
+): HydrateEmbeddedAction => {
+ const key = slice.slice_id;
+
+ return {
+ type: HYDRATE_DASHBOARD,
+ data: {
+ charts: {
+ [key]: {
+ ...chart,
+ id: key,
+ form_data: applyDefaultFormData(slice.form_data),
+ },
+ },
+ sliceEntities: {
+ slices: {
+ [key]: {
+ slice_id: key,
+ slice_url: slice.slice_url,
+ slice_name: slice.slice_name,
+ form_data: slice.form_data,
+ viz_type: slice.form_data.viz_type,
+ datasource: slice.form_data.datasource,
+ description: slice.description,
+ description_markeddown: slice.description_markeddown,
Review Comment:
**Suggestion:** The chart stack reads `description_markdown`, but this
payload stores `description_markeddown`, so expanded chart descriptions are
always missing in embedded charts. [api mismatch]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ed35e0364c4b4ee9ae0d5224e9917e26&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=ed35e0364c4b4ee9ae0d5224e9917e26&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/embedded/embeddedChart/hydrateEmbedded.ts
**Line:** 110:110
**Comment:**
*Api Mismatch: The chart stack reads `description_markdown`, but this
payload stores `description_markeddown`, so expanded chart descriptions are
always missing in embedded charts.
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%2F43769&comment_hash=04b535f25059131bbd767fe8e7e2421ad60303c85523452388bda859c0348606&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43769&comment_hash=04b535f25059131bbd767fe8e7e2421ad60303c85523452388bda859c0348606&reaction=dislike'>๐</a>
##########
superset-frontend/src/dashboard/components/EmbeddedModal/index.tsx:
##########
@@ -59,15 +62,19 @@ const ButtonRow = styled.div`
justify-content: flex-end;
`;
-export const DashboardEmbedControls = ({ dashboardId, onHide }: Props) => {
+export const DashboardEmbedControls = ({
+ dashboardId,
+ resourceType = 'dashboard',
+ onHide,
+}: Props) => {
const { addInfoToast, addDangerToast } = useToasts();
const [ready, setReady] = useState(true); // whether we have initialized yet
const [loading, setLoading] = useState(false); // whether we are currently
doing an async thing
const [embedded, setEmbedded] = useState<EmbeddedDashboard | null>(null); //
the embedded dashboard config
const [allowedDomains, setAllowedDomains] = useState<string>('');
const [showDeactivateConfirm, setShowDeactivateConfirm] = useState(false);
- const endpoint = `/api/v1/dashboard/${dashboardId}/embedded`;
+ const endpoint = `/api/v1/${resourceType}/${dashboardId}/embedded`;
Review Comment:
**Suggestion:** Changing `resourceType` without changing `dashboardId` does
not rerun the configuration fetch, so the modal shows and updates the previous
resource's embedding settings. [stale reference]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=71536d05aa884710a02ccee781997d30&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=71536d05aa884710a02ccee781997d30&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/EmbeddedModal/index.tsx
**Line:** 77:77
**Comment:**
*Stale Reference: Changing `resourceType` without changing
`dashboardId` does not rerun the configuration fetch, so the modal shows and
updates the previous resource's embedding settings.
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%2F43769&comment_hash=ea6edf4f64acff86eef083e001938d0d68b549297d581a29f9205a8a0ccc5cdc&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43769&comment_hash=ea6edf4f64acff86eef083e001938d0d68b549297d581a29f9205a8a0ccc5cdc&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]
