feat(embedded): embed a single chart as an independent entity [SIP-145] - #43769
Open
amaannawab923 wants to merge 21 commits into
Open
feat(embedded): embed a single chart as an independent entity [SIP-145]#43769amaannawab923 wants to merge 21 commits into
amaannawab923 wants to merge 21 commits into
Conversation
github-actions
Bot
added
risk:db-migration
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: a658e8b | Previous: 4a18556 | Ratio |
|---|---|---|---|
embedded entrypoint (JS) |
9322462 bytes |
7968217 bytes |
1.17 |
This comment was automatically generated by workflow using github-action-benchmark.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43769 +/- ##
==========================================
- Coverage 79.39% 79.34% -0.05%
==========================================
Files 2894 2898 +4
Lines 167749 167949 +200
Branches 38853 38892 +39
==========================================
+ Hits 133182 133267 +85
- Misses 32066 32180 +114
- Partials 2501 2502 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
amaannawab923
force-pushed
the
feat/embedded-chart
branch
from
September 3, 2026 05:50
dde47c2 to
874def3
Compare
Mirrors EmbeddedDashboard so both embeddable resource types share the same guest-token and allowed-domain semantics, including guest_token_revoked_before.
…oints Adds GET/POST/PUT/DELETE on /api/v1/chart/<pk>/embedded, mirroring the dashboard embedded endpoints, with a set_embedded permission for writes.
The embed view now falls back to EmbeddedChartDAO and passes resource_type plus chart_id into the bootstrap payload.
Builds the minimum slice of dashboard state a single chart needs and renders the existing dashboard Chart component against it, so cross-filtering, drill and the header controls work without reimplementation. Reuses HYDRATE_DASHBOARD rather than adding a parallel action, so no dashboard reducer changes are required; datasources is populated through its own setDatasources action.
Generalises the embed modal with an optional resourceType (defaulting to dashboard, so existing call sites are unchanged) and adds an Embed chart item to the chart header menu, gated on EMBEDDED_SUPERSET and the can_set_embedded permission on Chart.
t moved to @apache-superset/core/translation, css/styled to @apache-superset/core/theme, and ErrorBoundary is a named export.
Adds a CHART guest token resource type, validates it against the embedded chart uuid, and lets a directly-embedded chart satisfy the guest branch of raise_for_access without belonging to a dashboard.
Two gaps surfaced once a chart was actually rendered on its own: - The nativeFilters and dashboardLayout reducers dereference their slice unconditionally on HYDRATE_DASHBOARD, so the fabricated state has to carry both even though a lone chart has no layout tree and no native filters. - raise_for_access gates guest datasource access entirely on a dashboardId in the form data, which a standalone chart never has. Added a chart leg that authorizes the datasource when the guest token was issued for that chart and the request targets the chart's own datasource.
The modal is shared with the dashboard flow and its copy was hardcoded to 'dashboard', so a chart embed read 'This dashboard is ready to embed'.
same_origin parses the referrer eagerly, so an attacker-controlled value whose authority looks like it carries a non-numeric port (for example http://localhost:3007.evil.com/) raised ValueError instead of returning False. Access already failed closed, but any anonymous client could turn one header into a stack trace. This is not specific to embedded charts: the same path serves embedded dashboards today.
The dashboard gives every chart a holder element that owns two things the header controls reach for: the node handed to requestFullscreen, and the dashboard-chart-id-<id> class the jpeg and PDF exports select on. Rendering Chart directly meant neither existed, so fullscreen always reported 'not supported in this browser' and the image exports silently produced nothing. The embed now supplies its own holder rather than adopting ChartHolder, which would drag in drag-and-drop, resize and edit-mode logic an iframe has no use for. Popups are portaled into the fullscreen element for the same reason the dashboard does it: only that subtree is painted, so the header menu would otherwise be unreachable while fullscreen.
useIsMobile matches a media query against the current viewport, which inside an iframe is the size the host chose for the embed rather than the size of the device. A narrow embed on a desktop was therefore treated as a phone and lost its chart controls entirely, since SliceHeader hides them when isMobile. Mobile consumption mode is a whole-app experience (route guarding, drawer navigation) that an embed does not have to begin with.
Runs the real dashboard reducers against the real payload, so a slice that a HYDRATE_DASHBOARD handler dereferences without optional chaining cannot go missing again. That failure only surfaces at runtime and only in the embedded path, which is why it went unnoticed. Also fills in dashboardLayout and nativeFilters on HydrateEmbeddedAction, which had gone stale.
The chart title and the header menu rendered flush against the iframe edge, because on a dashboard that breathing room comes from the grid gutter and an embed has no grid. The holder supplies it instead. Sizing moved from the viewport to the holder's content box, so the chart lays out inside that padding rather than overflowing it, and the holder also carries dashboard-component-chart-holder, which is the class the fullscreen styles select on.
Import ordering and formatting from ruff, and dropping an unused alembic op import from the migration, which builds its table through the shared helpers.
tsc rejected passing the loosely typed dataset to setDatasources, which stores it as a Datasource. The explore endpoint returns the full datasource, so the field is declared as one rather than cast at the call site. Also applies oxfmt formatting the frontend hook expects.
The purge registry requires every inbound foreign key to a supported model to be declared, so adding embedded_charts.slice_id left Slice with an incomplete policy. Declared as OWNED with its three outbound keys, mirroring how embedded_dashboards is declared against Dashboard, which matches the cascade already on the relationship and the migration.
…mission set_embedded is exposed for POST and PUT but only documented post, so the generated spec carried a put operation with no responses and failed OpenAPI validation. The chart info permission set also needed can_set_embedded, which the dashboard equivalent already lists.
Two unit test failures, one real and one a fixture gap. has_embedded_chart_access queried the slice before establishing that a guest user exists, so it issued a database lookup on every datasource check that reached it, including paths where the grant could never hold. Resolving the guest user first skips the query entirely in that case. The embedded guest chart test builds its security manager as a mock specced against the real class, so has_guest_access_to_chart returned a truthy mock and the denial assertion no longer held. It is closed by default alongside the other paths the helper already closes, and the chart-direct grant now has a test of its own rather than only being exercised implicitly.
The embedded endpoints reference EmbeddedChartResponseSchema by $ref, but it was never added to openapi_spec_component_schemas, so the generated spec pointed at a component that did not exist and OpenAPI validation failed. The dashboard API registers its equivalent the same way. The config schema needs no entry, since it is inlined rather than referenced.
Master gained migrations while this branch was open, so down_revision pointed at a revision that is no longer the tip and the chain had two heads.
amaannawab923
force-pushed
the
feat/embedded-chart
branch
from
September 3, 2026 06:47
b78469b to
a658e8b
Compare
amaannawab923
marked this pull request as ready for review
September 3, 2026 08:18
amaannawab923
changed the title
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+2007
to
+2012
| class EmbeddedChartResponseSchema(Schema): | ||
| uuid = fields.String() | ||
| allowed_domains = fields.List(fields.String()) | ||
| chart_id = fields.String() | ||
| changed_on = fields.DateTime() | ||
| changed_by = fields.Nested(UserSchema) |
Contributor
There was a problem hiding this comment.
Suggestion: EmbeddedChart stores the identifier as slice_id, not chart_id, so dumping this schema omits the chart identifier from embedded-chart API responses. [api mismatch]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/schemas.py
**Line:** 2007:2012
**Comment:**
*Api Mismatch: `EmbeddedChart` stores the identifier as `slice_id`, not `chart_id`, so dumping this schema omits the chart identifier from embedded-chart API responses.
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
Contributor
|
The issue is correct. The class EmbeddedChartResponseSchema(Schema):
uuid = fields.String()
allowed_domains = fields.List(fields.String())
slice_id = fields.String()
changed_on = fields.DateTime()
changed_by = fields.Nested(UserSchema)I have reviewed the other comments in this PR. Would you like me to fetch all of them, validate their correctness, and implement fixes for the rest as well? superset/charts/schemas.py |
| 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) |
Contributor
There was a problem hiding this 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
Prompt for AI Agent 🤖
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
Comment on lines
+182
to
+184
| embedded: EmbeddedChart = ( | ||
| chart.embedded[0] if chart.embedded else EmbeddedChart() | ||
| ) |
Contributor
There was a problem hiding this 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
Prompt for AI Agent 🤖
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
| const [showDeactivateConfirm, setShowDeactivateConfirm] = useState(false); | ||
|
|
||
| const endpoint = `/api/v1/dashboard/${dashboardId}/embedded`; | ||
| const endpoint = `/api/v1/${resourceType}/${dashboardId}/embedded`; |
Contributor
There was a problem hiding this 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
Prompt for AI Agent 🤖
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| viz_type: slice.form_data.viz_type, | ||
| datasource: slice.form_data.datasource, | ||
| description: slice.description, | ||
| description_markeddown: slice.description_markeddown, |
Contributor
There was a problem hiding this 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
Prompt for AI Agent 🤖
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
Contributor
Code Review Agent Run #3cd749Actionable Suggestions - 0Additional Suggestions - 8
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
the
requires:rebase
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Screen.Recording.2026-09-01.at.9.38.38.PM.mov
Adds the ability to embed a single chart as an independent entity, rather than only
whole dashboards. A chart gets its own embed UUID, its own guest token scoped to that
chart alone, and its own allowed-domain list, and is served through the existing
/embedded/<uuid>route.This reimplements the approach from #33424 against current
master. That PR has beenopen since May 2025 and has drifted; this branch is a fresh implementation rather than a
rebase, and #33424 is left open and untouched for reference.
Approach. An embedded chart renders through the existing dashboard chart stack —
gridComponents/Chart, its header controls, drill and cross-filter plumbing — bysynthesising the minimum slice of dashboard state a single chart needs. That is what keeps
the header menu, drill,
View query,View as table, exports and fullscreen working withno reimplementation.
Notably this reuses
HYDRATE_DASHBOARDrather than introducing a parallelHYDRATE_EMBEDDEDaction, so no dashboard reducer is modified.charts,sliceEntities,dataMask,dashboardInfoanddashboardStatealready handle it;dashboardLayoutandnativeFiltershandle it too but dereference their slice withoutoptional chaining, so the fabricated payload carries an empty stand-in for each.
datasourceshas no hydrate handler at all and is populated through its own action.Backend
EmbeddedChartmodel and migration (a1c7e4b62f18), mirroringEmbeddedDashboardincluding
guest_token_revoked_beforeand allowed-domain semanticsEmbeddedChartDAOandGET/POST/DELETEon/api/v1/chart/<pk>/embedded/embedded/<uuid>resolves a UUID as either an embedded dashboard or an embedded chartCHARTguest-token resource type, withhas_guest_access_to_chartmatching on theembed UUID
raise_for_accesspreviously gated guest datasource access entirely on adashboardIdin the form data, which a standalone chart never has. Adds a chart leg that authorises
the datasource only when the guest token was issued for that chart and the request
targets that chart's own datasource.
Frontend
src/embedded/embeddedChart/— fabricated-state hydration, the explore-data hook, andthe wrapper that renders the dashboard
ChartEmbed chartin the chart header menu, gated oncan_set_embeddedonChartTwo fixes that fall out of this, both of which stand on their own:
superset/embedded/view.pyreturned a 500 rather than a 403 for a malformedReferer(for example a host that looks like it carries a non-numeric port). Access already
failed closed, so this is not a bypass, but any anonymous client could turn one header
into a stack trace. This affects embedded dashboards on
mastertoday and isisolated in its own commit so it can be taken separately.
useIsMobilematches a media query against the current viewport, which inside an iframeis the size the host chose for the embed rather than the size of the device. A narrow
embed on a desktop was served the phone experience and lost its chart controls entirely.
Gated behind
MOBILE_CONSUMPTION_MODE, which is off by default.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: a chart could only be embedded by embedding the dashboard that contained it.
After: charts embedded individually into a third-party page that Superset knows nothing
about — no dashboard behind them, each with its own UUID and guest token, header controls
and interactions intact. Screenshots to follow.
TESTING INSTRUCTIONS
EMBEDDED_SUPERSETand set a realGUEST_TOKEN_JWT_SECRET.superset db upgrade(createsembedded_charts).menu. Set an allowed domain and copy the UUID.
POST /api/v1/security/guest_token/with{"resources": [{"type": "chart", "id": "<uuid>"}], "user": {...}, "rls": []}@superset-ui/embedded-sdk,passing that UUID and a
fetchGuestTokenthat returns the token above.refresh, Enter fullscreen, View query, View as table and Download.
Worth verifying explicitly:
/embedded/<uuid>with noReferer, or one outside the allow-list, returns403; the allowed origin returns 200.
charts share a datasource — the check is on the embed UUID, not the dataset.
hydrateEmbedded.test.tsruns the real dashboard reducers against the real fabricatedpayload, so a slice that a
HYDRATE_DASHBOARDhandler dereferences cannot silently gomissing again; that failure surfaces only at runtime and only in the embedded path.
ADDITIONAL INFORMATION
EMBEDDED_SUPERSETThe migration only creates a new table and adds no column to an existing one, so it is
additive and reversible. Raising as a draft for early feedback, particularly on the
HYDRATE_DASHBOARDreuse and on whether theraise_for_accesschart leg is scopedtightly enough.